FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix tests that check error messages by cjihrig · Pull Request #3727 · nodejs/node · GitHub

/ node Public

test: fix tests that check error messages - #3727

Closed
cjihrig wants to merge 1 commit into
nodejs:masterfrom
cjihrig:errs
ConversationCommits (1)ChecksFiles changed
Closed

test: fix tests that check error messages#3727
cjihrig wants to merge 1 commit into
nodejs:masterfrom
cjihrig:errs

Conversation

cjihrig commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

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
f730782 
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

Copy link
Copy Markdown
Contributor

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

silverwind commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

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

silverwind reviewed Nov 9, 2015
View reviewed changes
Comment thread lib/path.js

silverwind Nov 9, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

cjihrig Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

silverwind Nov 12, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

silverwind added the test Issues and PRs related to the tests. label Nov 9, 2015

jbergstroem commented Nov 10, 2015

Copy link
Copy Markdown
Member

LGTM

1 similar comment

jasnell commented Nov 10, 2015

Copy link
Copy Markdown
Member

LGTM

thefourtheye reviewed Nov 10, 2015
View reviewed changes
Comment thread test/parallel/test-buffer.js

thefourtheye Nov 10, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why ignore case is necessary in these two places?

silverwind Nov 10, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

thefourtheye Nov 10, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

silverwind Nov 10, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'll do that on the landing.

cjihrig Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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
2c33819 
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 commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

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 closed this Nov 10, 2015
cjihrig deleted the errs branch November 10, 2015 15:44

jasnell commented Nov 10, 2015

Copy link
Copy Markdown
Member

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

cjihrig commented Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

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

silverwind added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 12, 2015

silverwind commented Nov 12, 2015

Copy link
Copy Markdown
Contributor

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

This was referenced Nov 12, 2015
Closed
Closed
Fishrock123 mentioned this pull request Nov 26, 2015
Closed
Fishrock123 mentioned this pull request Dec 15, 2015
Closed
jasnell mentioned this pull request Mar 17, 2016
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

silverwind

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL