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: fix error message check #5986

Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 1, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

test

Description of change

Fixes an errant error message test.

/cc @Fishrock123

@jasnell jasnell added the test Issues and PRs related to the tests. label Apr 1, 2016
@Trott
Copy link
Member

Trott commented Apr 1, 2016

Might as well fix the other instance of this in the file (for 'missing path') while you're in there, yeah?

Just in case it's not clear, what's going on here is that if the second argument is a string, it is the message that is given when the assertion fires. If the second argument is a RegExp, then it is used to validate the message from the thrown exception.

@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Apr 1, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Apr 1, 2016

LGTM and +1 to @Trott's suggestion.

@jasnell jasnell force-pushed the fix-test-module-loading-error-check branch from 884692f to 4cbcf65 Compare April 2, 2016 03:41
@jasnell
Copy link
Member Author

jasnell commented Apr 2, 2016

@jasnell
Copy link
Member Author

jasnell commented Apr 3, 2016

CI is green.

@@ -247,12 +247,12 @@ assert.deepEqual(children, {
assert.throws(function() {
console.error('require non-string');
require({ foo: 'bar' });
}, 'path must be a string or Buffer');
}, /path must be a string/);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more lenient. The test will still pass if the old message is returned right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but for this particular one, the message never included the or Buffer, that was a mis-edit that was missed when the buffers-in-fs pr was landed.

Copy link
Member

Choose a reason for hiding this comment

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

@thefourtheye Just in case there's a misunderstanding: This is not more lenient than the current test, although it appears more lenient thanks to the unfortunate way the assert.throws() API works.

The way it is without this PR, any throw will result in the test passing. The string ('path must be a string or Buffer') is not checked at all. Instead, that string is used as the message provided by the AssertionError that fires if the function does not throw. Fun, right?

Changing it to a RegExp the way this PR does means that it will be used to confirm the message in the Error thrown by the function, which is probably what was intended in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

PR to document that pitfall: #6029

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, Thanks :-)

jasnell added a commit that referenced this pull request Apr 4, 2016
PR-URL: #5986
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Apr 4, 2016

Landed in f739a12

@jasnell jasnell closed this Apr 4, 2016
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
PR-URL: #5986
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

@jasnell lts?

@MylesBorins
Copy link
Contributor

adding watch label.. /cc @jasnell

MylesBorins pushed a commit that referenced this pull request Jun 1, 2016
PR-URL: #5986
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 23, 2016
PR-URL: #5986
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
PR-URL: #5986
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jun 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants