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.

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

doc: list macOS as supporting filename argument #13111

Closed
wants to merge 3 commits into
base: master
from

Conversation

@chris--young
Contributor

chris--young commented May 19, 2017

macOS is not currently listed as a supported os for the fs.watch filename argument, but it does work. This commit updates the documentation to reflect that.

Fixes: #13108

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

docs, tests

@refack

This comment has been minimized.

Show comment
Hide comment
Member

refack commented May 19, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 19, 2017

Member

@chris--young thank you very much for the contribution 🥇

It would be extra beneficial if you tried to add some test cases that validate that the filename is indeed passed to the callback. That way we could run automated testing that validate the doc.
IMHO a good file to add the tests to is test/parallel/test-fs-watchfile.js, and you could use the helpers in our common test module to distunguish between the plafroms.

Member

refack commented May 19, 2017

@chris--young thank you very much for the contribution 🥇

It would be extra beneficial if you tried to add some test cases that validate that the filename is indeed passed to the callback. That way we could run automated testing that validate the doc.
IMHO a good file to add the tests to is test/parallel/test-fs-watchfile.js, and you could use the helpers in our common test module to distunguish between the plafroms.

@gireeshpunathil

This comment has been minimized.

Show comment
Hide comment
@gireeshpunathil

gireeshpunathil May 19, 2017

Member

AIX too please.

bash-4.3$ uname   
AIX
bash-4.3$ cat w.js
var fs = require('fs');
fs.watch('myfile', (eventType, filename) => {
  console.log(`event type is: ${eventType}`);
  if (filename) {
    console.log(`filename provided: ${filename}`);
  } else {
    console.log('filename not provided');
  }
});
bash-4.3$ node w.js &
[1] 1769476
bash-4.3$ echo hello > myfile
bash-4.3$ event type is: change
filename provided: myfile
event type is: change
filename provided: myfile
Member

gireeshpunathil commented May 19, 2017

AIX too please.

bash-4.3$ uname   
AIX
bash-4.3$ cat w.js
var fs = require('fs');
fs.watch('myfile', (eventType, filename) => {
  console.log(`event type is: ${eventType}`);
  if (filename) {
    console.log(`filename provided: ${filename}`);
  } else {
    console.log('filename not provided');
  }
});
bash-4.3$ node w.js &
[1] 1769476
bash-4.3$ echo hello > myfile
bash-4.3$ event type is: change
filename provided: myfile
event type is: change
filename provided: myfile
@gireeshpunathil

Per my comment - will you please add AIX also into the list?

Show outdated Hide outdated test/parallel/test-fs-watchfile.js
@@ -63,3 +63,25 @@ fs.watchFile(enoentFile, {interval: 0}, common.mustCall(function(curr, prev) {
fs.unwatchFile(enoentFile);
}
}, 2));
// Watch events should callback with a filename
if (common.isLinux || common.isOSX) {

This comment has been minimized.

@aashil

aashil May 19, 2017

Member

I would add Windows too. You can also add AIX according to @gireeshpunathil's suggestion.

@aashil

aashil May 19, 2017

Member

I would add Windows too. You can also add AIX according to @gireeshpunathil's suggestion.

This comment has been minimized.

@refack

refack May 19, 2017

Member

I just checked, it passes on windows.

@refack

refack May 19, 2017

Member

I just checked, it passes on windows.

Show outdated Hide outdated test/parallel/test-fs-watchfile.js
@@ -63,3 +63,25 @@ fs.watchFile(enoentFile, {interval: 0}, common.mustCall(function(curr, prev) {
fs.unwatchFile(enoentFile);
}
}, 2));
// Watch events should callback with a filename
if (common.isLinux || common.isOSX) {

This comment has been minimized.

@refack

refack May 19, 2017

Member

I just checked, it passes on windows.

@refack

refack May 19, 2017

Member

I just checked, it passes on windows.

Show outdated Hide outdated test/parallel/test-fs-watchfile.js
const dir = common.tmpDir + '/watch';
fs.mkdir(dir, common.mustCall(function(err) {
assert(!err);

This comment has been minimized.

@refack

refack May 19, 2017

Member

If you could use assert.ifError if (err) assert.fail(err); instead. It will rethrow the error so we know what went wrong.
assert(!err) will just say true != false

@refack

refack May 19, 2017

Member

If you could use assert.ifError if (err) assert.fail(err); instead. It will rethrow the error so we know what went wrong.
assert(!err) will just say true != false

Show outdated Hide outdated test/parallel/test-fs-watchfile.js
fs.watch(dir, common.mustCall(function(eventType, filename) {
this._handle.close();
common.refreshTmpDir();

This comment has been minimized.

@refack

refack May 19, 2017

Member

For the test to pass on Windows you should remove the common.refreshTmpDir();. Don't worry about it, the test harness will take care of it.

@refack

refack May 19, 2017

Member

For the test to pass on Windows you should remove the common.refreshTmpDir();. Don't worry about it, the test harness will take care of it.

Show outdated Hide outdated test/parallel/test-fs-watchfile.js
}));
fs.writeFile(`${dir}/foo.txt`, 'foo', common.mustCall((err) => {
if (err) {

This comment has been minimized.

@refack

refack May 19, 2017

Member

you can replace the whole callback with (err) => assert.ifError(err)
Ref: #13092 (comment)

@refack

refack May 19, 2017

Member

you can replace the whole callback with (err) => assert.ifError(err)
Ref: #13092 (comment)

This comment has been minimized.

@refack

refack May 19, 2017

Member

you should replace the whole callback with common.mustCall((err) => {if (err) assert.fail(err)})
Ref: #13115

@refack

refack May 19, 2017

Member

you should replace the whole callback with common.mustCall((err) => {if (err) assert.fail(err)})
Ref: #13115

@refack

This comment has been minimized.

Show comment
Hide comment
@refack
Member

refack commented May 19, 2017

@chris--young thanks

@refack

refack approved these changes May 19, 2017

@refack refack self-assigned this May 19, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@chris--young

This comment has been minimized.

Show comment
Hide comment
@chris--young

chris--young May 19, 2017

Contributor

Any idea what's up with that failed windows-fanned test? Seeing this in the logs...

node-test-binary-windows » 1,vs2015-x86,win2008r2 completed with result FAILURE

I don't have a Windows box to test on right now, should be able to setup a vm over the weekend if needed.

Contributor

chris--young commented May 19, 2017

Any idea what's up with that failed windows-fanned test? Seeing this in the logs...

node-test-binary-windows » 1,vs2015-x86,win2008r2 completed with result FAILURE

I don't have a Windows box to test on right now, should be able to setup a vm over the weekend if needed.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 19, 2017

Member

Any idea what's up with that failed windows-fanned test?

Ignore, it's a infrastructure fail. Also the linux fan is backlogged.
You can consider it green 👍

Member

refack commented May 19, 2017

Any idea what's up with that failed windows-fanned test?

Ignore, it's a infrastructure fail. Also the linux fan is backlogged.
You can consider it green 👍

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 19, 2017

Member

@chris--young if you're not aware of our landing procedure; now that your part is done, we keep the PR open for 48-72 hours so that anyone who might want to voice an opinion will have a chance.
Ref: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-8-landing
If it all goes smoothly I'll land this on Monday.

Thanks again!

Member

refack commented May 19, 2017

@chris--young if you're not aware of our landing procedure; now that your part is done, we keep the PR open for 48-72 hours so that anyone who might want to voice an opinion will have a chance.
Ref: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-8-landing
If it all goes smoothly I'll land this on Monday.

Thanks again!

@chris--young

This comment has been minimized.

Show comment
Hide comment
@chris--young

chris--young May 19, 2017

Contributor

sweet

Contributor

chris--young commented May 19, 2017

sweet

Show outdated Hide outdated test/parallel/test-fs-watchfile.js
const dir = common.tmpDir + '/watch';
fs.mkdir(dir, common.mustCall(function(err) {
if (err)

This comment has been minimized.

@lpinca

lpinca May 20, 2017

Member

Nit: can you please replace this with assert.ifError()?

Edit: Just read @refack comments against assert.ifError(). It is actually widely used in our tests but what is used here is also ok so feel free to ignore this.

@lpinca

lpinca May 20, 2017

Member

Nit: can you please replace this with assert.ifError()?

Edit: Just read @refack comments against assert.ifError(). It is actually widely used in our tests but what is used here is also ok so feel free to ignore this.

This comment has been minimized.

@refack

refack May 20, 2017

Member

Edit: Just read @refack comments against assert.ifError(). It is actually widely used in our tests but what is used here is also ok so feel free to ignore this.

Vote for a better ifError #12820 ✌️

@refack

refack May 20, 2017

Member

Edit: Just read @refack comments against assert.ifError(). It is actually widely used in our tests but what is used here is also ok so feel free to ignore this.

Vote for a better ifError #12820 ✌️

@gireeshpunathil

@chris--young - thanks! I am fine with the changes.

@aqrln

Thanks for your contribution! Can you please change the message of the first commit to use doc:, not docs:? That can be fixed by whoever lands the commit as well, though.

@chris--young chris--young changed the title from docs: list macOS as supporting filename argument to doc: list macOS as supporting filename argument May 21, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack
Member

refack commented May 22, 2017

CI: https://ci.nodejs.org/job/node-test-commit/10067/
/cc @lpinca @aqrln, where your comments addressed?

@lpinca

lpinca approved these changes May 22, 2017

Show outdated Hide outdated test/parallel/test-fs-watchfile.js
const dir = common.tmpDir + '/watch';
fs.mkdir(dir, common.mustCall(function(err) {
if (err)

This comment has been minimized.

@aqrln

aqrln May 22, 2017

Member

I'd prefer it to be either a one-liner or to have braces, but maybe it's just me, and it clearly does not violate the code style this way too, so you can ignore this comment :)

@aqrln

aqrln May 22, 2017

Member

I'd prefer it to be either a one-liner or to have braces, but maybe it's just me, and it clearly does not violate the code style this way too, so you can ignore this comment :)

This comment has been minimized.

@cjihrig

cjihrig May 22, 2017

Contributor

I really think we should use assert.ifError(err) in these cases.

@cjihrig

cjihrig May 22, 2017

Contributor

I really think we should use assert.ifError(err) in these cases.

This comment has been minimized.

@refack

refack May 22, 2017

Member

+1
Either one liner or braces.

@refack

refack May 22, 2017

Member

+1
Either one liner or braces.

@aqrln

aqrln approved these changes May 22, 2017

Show outdated Hide outdated test/parallel/test-fs-watchfile.js
fs.writeFile(`${dir}/foo.txt`, 'foo', common.mustCall(function(err) {
if (err)
assert.fail(err);

This comment has been minimized.

@aqrln

aqrln May 22, 2017

Member

Ditto

@aqrln

aqrln May 22, 2017

Member

Ditto

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 22, 2017

Member

@chris--young we raised a code style nit, then this will land.

Member

refack commented May 22, 2017

@chris--young we raised a code style nit, then this will land.

@aashil

aashil approved these changes May 22, 2017

@chris--young

This comment has been minimized.

Show comment
Hide comment
@chris--young

chris--young May 22, 2017

Contributor

@refack made those changes. It looks like a new test is failing now, possibly another infrastructure fail

Checking out Revision 21535c38a3f800117089872f890e9d1f336024e0 (refs/remotes/origin/_jenkins_local_branch)
 > git config core.sparsecheckout # timeout=10
 > git checkout -f 21535c38a3f800117089872f890e9d1f336024e0
FATAL: Could not checkout 21535c38a3f800117089872f890e9d1f336024e0
hudson.plugins.git.GitException: Command "git checkout -f 21535c38a3f800117089872f890e9d1f336024e0" returned status code 128:
stdout: 
stderr: fatal: reference is not a tree: 21535c38a3f800117089872f890e9d1f336024e0
Contributor

chris--young commented May 22, 2017

@refack made those changes. It looks like a new test is failing now, possibly another infrastructure fail

Checking out Revision 21535c38a3f800117089872f890e9d1f336024e0 (refs/remotes/origin/_jenkins_local_branch)
 > git config core.sparsecheckout # timeout=10
 > git checkout -f 21535c38a3f800117089872f890e9d1f336024e0
FATAL: Could not checkout 21535c38a3f800117089872f890e9d1f336024e0
hudson.plugins.git.GitException: Command "git checkout -f 21535c38a3f800117089872f890e9d1f336024e0" returned status code 128:
stdout: 
stderr: fatal: reference is not a tree: 21535c38a3f800117089872f890e9d1f336024e0
@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 22, 2017

Member

Ignore it fedora22 is obsolete anyway. I'm landing this.

Member

refack commented May 22, 2017

Ignore it fedora22 is obsolete anyway. I'm landing this.

refack added a commit to refack/node that referenced this pull request May 22, 2017

doc: list macOS as supporting filename argument
also added regression tests

PR-URL: nodejs#13111
Fixes: nodejs#13108
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 22, 2017

Member

Landed in c470f04

Member

refack commented May 22, 2017

Landed in c470f04

@refack refack closed this May 22, 2017

jasnell added a commit that referenced this pull request May 23, 2017

doc: list macOS as supporting filename argument
also added regression tests

PR-URL: #13111
Fixes: #13108
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

jasnell added a commit that referenced this pull request May 23, 2017

doc: list macOS as supporting filename argument
also added regression tests

PR-URL: #13111
Fixes: #13108
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment