test: fix error message check - #5986
Conversation
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
LGTM and +1 to @Trott's suggestion. |
Sorry, something went wrong.
Sorry, something went wrong.
|
CI is green. |
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
PR to document that pitfall: #6029
Sorry, something went wrong.
There was a problem hiding this comment.
Cool, Thanks :-)
Sorry, something went wrong.
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>
PR-URL: #5986 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #5986 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Pull Request check-list
this 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