GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: fix tests that check error messages by cjihrig · Pull Request #3727 · nodejs/node

Conversation

@cjihrig

@cjihrig 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@cjihrig
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

Fishrock123 commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
silverwind
silverwind reviewed Nov 9, 2015
View reviewed changes
Comment thread lib/path.js

@silverwind silverwind Nov 9, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@silverwind silverwind Nov 12, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@silverwind silverwind added the test Issues and PRs related to Node.js core tests and test infrastructure. label Nov 9, 2015
@jbergstroem

jbergstroem commented Nov 10, 2015

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1 similar comment
@jasnell

jasnell commented Nov 10, 2015

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
thefourtheye
thefourtheye reviewed Nov 10, 2015
View reviewed changes
Comment thread test/parallel/test-buffer.js

@thefourtheye thefourtheye Nov 10, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why ignore case is necessary in these two places?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@silverwind silverwind Nov 10, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@thefourtheye thefourtheye Nov 10, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@silverwind silverwind Nov 10, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'll do that on the landing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@silverwind silverwind self-assigned this Nov 10, 2015
cjihrig added a commit that referenced this pull request Nov 10, 2015
@cjihrig @silverwind
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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@silverwind

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind silverwind closed this Nov 10, 2015
@cjihrig
cjihrig deleted the errs branch November 10, 2015 15:44
@jasnell

jasnell commented Nov 10, 2015

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@silverwind silverwind added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 12, 2015
@silverwind

silverwind commented Nov 12, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Nov 12, 2015
Consistent error messages in all modules #3374
Closed
Rediscuss if error message changes are semver-major #3776
Closed
@Fishrock123 Fishrock123 mentioned this pull request Nov 26, 2015
deps: upgrade npm to 3.5.0 #4032
Closed
@Fishrock123 Fishrock123 mentioned this pull request Dec 15, 2015
v5.3.0 Release Proposal (Stable) #4281
Closed
@jasnell jasnell mentioned this pull request Mar 17, 2016
Planning for v6 #5766
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@silverwind silverwind

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@cjihrig @Fishrock123 @silverwind @jbergstroem @jasnell @thefourtheye
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.