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.

Sign up for GitHub

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

Jump to bottom

test: use error code rather than message in test #20859

Closed
wants to merge 1 commit into from
Closed

test: use error code rather than message in test #20859

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 21, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@Trott
test: use error code rather than message in test
a26aece 
Use err.code checking instead of err.message checking in
test-child-process-fork-closed-channel-segfault.
@Trott Trott added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels May 21, 2018
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 21, 2018
@Trott
Copy link
Member Author

Trott commented May 21, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/14991/

This test is currently failing a lot on Windows already, and I don't have a good reason to believe this will fix that, but let's see what happens, I guess.

BridgeAR
BridgeAR reviewed May 21, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

This should have zero impact on the test outcome.

@Trott
Copy link
Member Author

Trott commented May 21, 2018

This should have zero impact on the test outcome.

@BridgeAR Yes, that's my expectation. I wrote all that stuff about how the test is currently failing not because I expect this to fix it, but because I expect people (who aren't aware that the test is already failing more often than passing on Windows) to (understandably) think that this PR introduces a problem when it doesn't.

This PR is a style improvement that also makes the test less brittle, but it does not address the test's current issues. If it ends up somehow mysteriously fixing the test's current problems, that will be unexpected for sure.

@Trott
Copy link
Member Author

Trott commented May 21, 2018

Unrelated http2 test failures, re-running:

LinxONE re-run: https://ci.nodejs.org/job/node-test-commit-linuxone/1459/
FreeBSD re-run: https://ci.nodejs.org/job/node-test-commit-freebsd/17910/

Might be futile until the test or underlying bug is fixed (as it fails roughly 80% of the time now), but Windows re-run:

https://ci.nodejs.org/job/node-test-commit-windows-fanned/18143/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 21, 2018
@Trott
Copy link
Member Author

Trott commented May 21, 2018

FreeBSD re-run: https://ci.nodejs.org/job/node-test-commit-freebsd/17927/

@Trott
Copy link
Member Author

Trott commented May 21, 2018

FreeBSD is the only remaining platform needing green/yellow here: https://ci.nodejs.org/job/node-test-commit-freebsd/17929/

@Trott
Copy link
Member Author

Trott commented May 21, 2018

CI is all green/yellow now. Please 👍 here if you are in favor of fast-tracking.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label May 21, 2018
Trott added a commit to Trott/io.js that referenced this pull request May 22, 2018
@Trott
test: use error code rather than message in test
4f0ab76 
Use err.code checking instead of err.message checking in
test-child-process-fork-closed-channel-segfault.

PR-URL: nodejs#20859
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott
Copy link
Member Author

Trott commented May 22, 2018

Landed in 4f0ab76

@Trott Trott closed this May 22, 2018
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@Trott @MylesBorins
test: use error code rather than message in test
ce40c70 
Use err.code checking instead of err.message checking in
test-child-process-fork-closed-channel-segfault.

PR-URL: #20859
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax addaleax mentioned this pull request May 22, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@Trott @MylesBorins
test: use error code rather than message in test
a8c74e8 
Use err.code checking instead of err.message checking in
test-child-process-fork-closed-channel-segfault.

PR-URL: #20859
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott Trott deleted the message-to-code branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@trivikr trivikr trivikr approved these changes

@richardlau richardlau richardlau approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.