-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Conversation
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. |
LGTM and +1 to @Trott's suggestion. |
884692f
to
4cbcf65
Compare
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/); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, Thanks :-)
PR-URL: #5986 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Landed in f739a12 |
PR-URL: #5986 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell lts? |
adding watch label.. /cc @jasnell |
PR-URL: #5986 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #5986 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #5986 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Pull Request check-list
make -j8 test
(UNIX) orvcbuild test nosign
(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
Affected core subsystem(s)
test
Description of change
Fixes an errant error message test.
/cc @Fishrock123