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. |
jasnell
force-pushed
the
fix-test-module-loading-error-check
branch
from
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.
This is more lenient. The test will still pass if the old message is returned right?
There was a problem hiding this comment.
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.
@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.
PR to document that pitfall: #6029
PR-URL: #5986 Reviewed-By: Colin Ihrig <[email protected]>
|
Landed in f739a12 |
PR-URL: #5986 Reviewed-By: Colin Ihrig <[email protected]>
|
@jasnell lts? |
|
adding watch label.. /cc @jasnell |
PR-URL: #5986 Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #5986 Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #5986 Reviewed-By: Colin Ihrig <[email protected]>
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