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

fs.watch error message includes filename #2748

Closed
wants to merge 1 commit into from
Closed

fs.watch error message includes filename #2748

wants to merge 1 commit into from

Conversation

charlierudolph
Copy link

Brought over from nodejs/node-v0.x-archive#25542

@brendanashworth brendanashworth added the fs Issues and PRs related to the fs subsystem / file system. label Sep 9, 2015
cjihrig
cjihrig reviewed Sep 9, 2015
View reviewed changes
lib/fs.js
@@ -1213,7 +1213,9 @@ function FSWatcher() {
this._handle.onchange = function(status, event, filename) {
if (status < 0) {
self._handle.close();
self.emit('error', errnoException(status, 'watch'));
var error = errnoException(status, 'watch ' + filename);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use let instead of var, and a template string instead of concatenation.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 9, 2015

You changed two code paths, but only added a test for one. Can you test for the other too?

@charlierudolph
Copy link
Author

Thanks for the review. Code comments addressed. I don't actually know how to test the other code path. which is an error occurring while watching a file. Any suggestions?

thefourtheye
thefourtheye reviewed Sep 9, 2015
View reviewed changes
test/parallel/test-fs-watch-error.js
assert(err);
assert(/non-existent-file/.test(err));
assert.equal(err.filename, 'non-existent-file');
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessary.

Copy link
Author

Choose a reason for hiding this comment

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

unfortunately it is. If the function doesn't return true, it throws the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

@charlierudolph Oops, sorry. My bad.

@sindresorhus
Copy link

Even if it's non-standard, might be worth going with camelCased fileName for consistency with other envs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/fileName

@cjihrig
Copy link
Contributor

cjihrig commented Sep 9, 2015

One important difference is that Error.prototype.fileName indicates the file that raised the exception, while the proposed addition in this PR indicates a parameter to a file system operation.

@charlierudolph
Copy link
Author

@cjihrig bump. Anything else this PR needs?

thefourtheye
thefourtheye reviewed Sep 16, 2015
View reviewed changes
lib/fs.js
@@ -1213,7 +1213,9 @@ function FSWatcher() {
this._handle.onchange = function(status, event, filename) {
if (status < 0) {
self._handle.close();
self.emit('error', errnoException(status, 'watch'));
let error = errnoException(status, `watch ${filename}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can actually be const

@cjihrig
Copy link
Contributor

cjihrig commented Sep 17, 2015

It LGTM, but I'd really like a test for the emit path. Would anyone object to a slightly artificial test like:

var watcher = fs.watch(__filename);
watcher._handle.onchange(-1, 'ENOENT', 'foo.js');

@charlierudolph
Copy link
Author

okay @cjihrig the emit path now has a test.

cjihrig
cjihrig reviewed Sep 29, 2015
View reviewed changes
test/sequential/test-fs-watch.js
});

var watcher = fs.watch(__filename);
watcher.on('error', function(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating watcherErrorCaught, you can just wrap this function in common.mustCall(). That way you can ensure it was called, and fail the test if it's not.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 29, 2015

One comment, but this LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Sep 29, 2015

Oh, and can you squash this down to a single commit?

@charlierudolph
Copy link
Author

Switched to using common.mustCall and squashed to a single commit.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 30, 2015

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

@charlierudolph
Copy link
Author

That build is very complex to traverse. Okay from the link provided I got to
https://ci.nodejs.org/job/node-test-binary-arm/88/RUN_SUBSET=5,nodes=pi1-raspbian-wheezy/tapTestReport/
which says test-stringbytes-external.js is what failed. I don't know how I could have made that fail, especially since no error message is provided.

The build page shows another commit is being tested in addition to mine.
https://ci.nodejs.org/job/node-test-binary-arm/88/

@thefourtheye
Copy link
Contributor

Don't worry about that test. It fails often.

@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 1, 2015
@cjihrig cjihrig mentioned this pull request Oct 1, 2015
Closed
cjihrig pushed a commit that referenced this pull request Oct 2, 2015
@cjihrig
fs: include filename in watch errors
87e820e 
This commit adds the relevant filename to fs.watch() errors.

Refs: nodejs/node-v0.x-archive#25542
PR-URL: #2748
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig
Copy link
Contributor

cjihrig commented Oct 2, 2015

Thanks! Landed in 87e820e

@cjihrig cjihrig closed this Oct 2, 2015
@jasnell jasnell mentioned this pull request Oct 8, 2015
Closed
29 tasks
jasnell pushed a commit that referenced this pull request Oct 8, 2015
@jasnell
fs: include filename in watch errors
4a35ba4 
This commit adds the relevant filename to fs.watch() errors.

Refs: nodejs/node-v0.x-archive#25542
PR-URL: #2748
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell jasnell mentioned this pull request Oct 8, 2015
Closed
@leonardoadame leonardoadame mentioned this pull request Jul 8, 2023
Open
@aliscco aliscco mentioned this pull request Jul 9, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@charlierudolph @cjihrig @sindresorhus @thefourtheye @jasnell @MylesBorins @brendanashworth