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

process: unify error message from chdir() errors #19088

Closed
wants to merge 2 commits into from
Closed

process: unify error message from chdir() errors #19088

wants to merge 2 commits into from

Conversation

SirR4T
Copy link
Contributor

@SirR4T SirR4T commented Mar 2, 2018 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Partly fixes #12351

Affected core subsystem(s)

process

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 2, 2018
@joyeecheung
Copy link
Member

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

richardlau
richardlau reviewed Mar 2, 2018
View reviewed changes
test/parallel/test-fs-chdir-errormessage.js Outdated
const process = require('process');
const assert = require('assert');

assert.throws(
Copy link
Member

Choose a reason for hiding this comment

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

Use common.expectsError instead?

cjihrig
cjihrig approved these changes Mar 2, 2018
View reviewed changes
test/parallel/test-fs-chdir-errormessage.js Outdated
'use strict';

require('../common');
const process = require('process');
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to require process

@SirR4T
Copy link
Contributor Author

SirR4T commented Mar 2, 2018

I don't understand why the test is failing, from this ci job link.

@richardlau
Copy link
Member

I don't understand why the test is failing, from this ci job link.

The test was recently modified by #18986 but the CI of that PR also failed the test on a different platform. I'm fairly sure it's unrelated the to the changes in this PR.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 2, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Mar 2, 2018

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2018
mcollina
mcollina approved these changes Mar 2, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina
Copy link
Member

mcollina commented Mar 2, 2018

PR with the fix for the broken test: #19093

@addaleax
Copy link
Member

addaleax commented Mar 2, 2018

Fwiw, I don’t think changes to the messages of errors that already have had errno-based error codes should have to be semver-major… @nodejs/tsc thoughts?

@joyeecheung
Copy link
Member

Fwiw, I don’t think changes to the messages of errors that already have had errno-based error codes should have to be semver-major… @nodejs/tsc thoughts?

I think @ljharb raised somewhere that we should still treat them as breaking changes until we have assigned .code to all errors thrown from Node.js core, the rationale is if all the errors don't have .code, then the user can not start to fully rely on them.

@joyeecheung
Copy link
Member

That being said, I think we can make an exception for libuv errors like this one because they are pretty old so should be well-recognized in the user land. Not so much about the ERR_* errors though.

@ljharb
Copy link
Member

ljharb commented Mar 3, 2018

If the error previously had a code, then it’s less risky to treat a message change as non-major, but the safest bet is waiting until there’s a version of node with zero codeless errors before doing so.

@joyeecheung
Copy link
Member

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

@apapirovski
Copy link
Member

Landed in b32bcf7

@apapirovski apapirovski closed this Mar 6, 2018
apapirovski pushed a commit that referenced this pull request Mar 6, 2018
@SirR4T @apapirovski
process: unify error message from chdir() errors
b32bcf7 
PR-URL: #19088
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@SirR4T SirR4T deleted the errorMessageUnification branch March 6, 2018 14:32
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@SirR4T @MayaLekova
process: unify error message from chdir() errors
0e9644b 
PR-URL: nodejs#19088
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@richardlau richardlau richardlau 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. c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ENOENT error message inconsistencies