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 tests that check error messages #3727

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@cjihrig
Copy link
Contributor

commented Nov 9, 2015

20285ad changed the format of error messages throughout lib. However, the tests were not updated to reflect these changes. This commit makes those changes.

test: fix tests that check error messages
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

This comment has been minimized.

Copy link
Member

commented Nov 9, 2015

@silverwind @micnic please be sure to always run the tests before landing. :D

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2015

Thanks @cjihrig for picking the slack! LGTM, verified locally as CI seems to still be down.

@@ -566,7 +566,7 @@ posix.extname = function(path) {
posix.format = function(pathObject) {
if (pathObject === null || typeof pathObject !== 'object') {
throw new TypeError(
"Parameter 'pathObject' must be an object, not " + typeof pathObject
'Parameter "pathObject" must be an object, not ' + typeof pathObject

This comment has been minimized.

Copy link
@silverwind

silverwind Nov 9, 2015

Contributor

Was this one missed in the original commit, by the way?

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 10, 2015

Author Contributor

Yes. And it made one of the path tests a pain to fix.

This comment has been minimized.

Copy link
@silverwind

silverwind Nov 12, 2015

Contributor

@micnic want to do a second pass if there are more errors that were missed, like this one?

@silverwind silverwind added the test label Nov 9, 2015

@jbergstroem

This comment has been minimized.

Copy link
Member

commented Nov 10, 2015

LGTM

1 similar comment
@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 10, 2015

LGTM


assert.throws(function() {
new Buffer(null);
}, /must start with number, buffer, array or string/);
}, /must start with number, buffer, array or string/i);

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 10, 2015

Contributor

Why ignore case is necessary in these two places?

This comment has been minimized.

Copy link
@silverwind

silverwind Nov 10, 2015

Contributor

The error string is 'Must start with number, buffer, array or string'.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 10, 2015

Contributor

Then, why not fix the string itself? That would be better, right?

This comment has been minimized.

Copy link
@silverwind

silverwind Nov 10, 2015

Contributor

I'll do that on the landing.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 10, 2015

Author Contributor

I made the regex case insensitive because if that were already the case, this test wouldn't have broken at all.

@silverwind silverwind self-assigned this Nov 10, 2015

cjihrig added a commit that referenced this pull request Nov 10, 2015

test: fix tests that check error messages
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 <me@silverwind.io>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2015

Landed in 2c33819 with the suggested regex change. master is green again, sorry for the trouble.

I'll have a look at the remaining error messages in case more were missed.

@silverwind silverwind closed this Nov 10, 2015

@cjihrig cjihrig deleted the cjihrig:errs branch Nov 10, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 10, 2015

@cjihrig ... should this be looked at for v4.x?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2015

I don't think so. The PR that updated the error messages was deemed not LTS (#3374).

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2015

This depends on #3374, which makes it a semver-major.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.