Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: improve module version mismatch error check #10636

Merged
merged 1 commit into from
Jan 9, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 5, 2017

Refs: #10606

Don't run the CI yet. The test will fail until #10606 lands.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 5, 2017
@mscdex mscdex added the addons Issues and PRs related to native addons. label Jan 5, 2017
'was compiled against a different Node.js version using\n' +
'NODE_MODULE_VERSION 42. This version of Node.js requires\n' +
`NODE_MODULE_VERSION ${process.versions.modules}.`);
`^Error: The module '.+'\n` +
Copy link
Contributor

@sam-github sam-github Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most node Error: messages don't insert line breaks into the message output.

Consider removing the newlines, so that the message can wrap at the user's terminal boundary, or in the case of stderr being directed to a log aggregation service, the entire error will be one single line/log entry.

I would make each sentence a single line, or maybe the entire thing two lines, with the second line being "Please try...".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is just improving the regular expression that matches the existing error message (which includes explicit line breaks). It's not changing the error message itself.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is ✅

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 6, 2017

Refs: nodejs#10606
PR-URL: nodejs#10636
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@cjihrig cjihrig merged commit 775de9c into nodejs:master Jan 9, 2017
@cjihrig cjihrig deleted the err-msg branch January 9, 2017 17:07
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@italoacasas
Copy link
Contributor

Since #10606 is a semver-major and these two commits need to be together, I'm adding do-not-land labels here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants