test: fix tests that check error messages#3727
Uh oh!
There was an error while loading. Please reload this page.
Conversation
20285ad changed the format of error messages throughout lib. However, the tests were not updated to reflect these changes. This commit makes those changes.
Fishrock123 commented Nov 9, 2015
@silverwind@micnic please be sure to always run the tests before landing. :D |
silverwind commented Nov 9, 2015
Thanks @cjihrig for picking the slack! LGTM, verified locally as CI seems to still be down. |
There was a problem hiding this comment.
Was this one missed in the original commit, by the way?
There was a problem hiding this comment.
Yes. And it made one of the path tests a pain to fix.
There was a problem hiding this comment.
@micnic want to do a second pass if there are more errors that were missed, like this one?
jbergstroem commented Nov 10, 2015
LGTM |
1 similar comment
jasnell commented Nov 10, 2015
LGTM |
There was a problem hiding this comment.
Why ignore case is necessary in these two places?
There was a problem hiding this comment.
The error string is 'Must start with number, buffer, array or string'.
There was a problem hiding this comment.
Then, why not fix the string itself? That would be better, right?
There was a problem hiding this comment.
I'll do that on the landing.
There was a problem hiding this comment.
I made the regex case insensitive because if that were already the case, this test wouldn't have broken at all.
20285ad changed the format of error messages throughout lib. However, the tests were not updated to reflect these changes. This commit makes those changes. PR-URL: #3727 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
silverwind commented Nov 10, 2015
Landed in 2c33819 with the suggested regex change. I'll have a look at the remaining error messages in case more were missed. |
jasnell commented Nov 10, 2015
@cjihrig ... should this be looked at for v4.x? |
cjihrig commented Nov 10, 2015
I don't think so. The PR that updated the error messages was deemed not LTS (#3374). |
silverwind added the semver-major silverwind commented Nov 12, 2015
This depends on #3374, which makes it a |
6 participants
20285ad changed the format of error messages throughout lib. However, the tests were not updated to reflect these changes. This commit makes those changes.