test,fs: test fs.watch for `filename` #13411

refack · 2017-06-02T19:17:28Z

Checklist

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

Affected core subsystem(s)

test,fs

refack · 2017-06-02T19:21:06Z

Stress: https://ci.nodejs.org/job/node-stress-single-test/1266/
Gate CI: https://ci.nodejs.org/job/node-test-commit-linuxone/6334/

addaleax · 2017-06-02T19:24:09Z

test/sequential/test-fs-watchfile.js

-      this._handle.close();
-      assert.strictEqual(filename, 'foo.txt');
-    }));
+function WatchTestCase(is, dirName, fileName, field) {


is is a very nondescriptive name… I’d probably use the reverse and call it skip.

shouldSkip Ok? I like predicates with predicate names.

addaleax · 2017-06-02T19:24:28Z

test/sequential/test-fs-watchfile.js

+  Object.defineProperties(this, {
+    dirPath: { get: () => `${common.tmpDir}/${this.dirName}` },
+    filePath: { get: () => `${this.dirPath}/${this.fileName}` }
+  });


Any reason to not turn this into an ES6 class and use fancy get dirPath() { … } syntax? :)

silly conservatism.
Will refactor.

addaleax · 2017-06-02T19:26:22Z

test/sequential/test-fs-watchfile.js

+  if (!testCase.is) continue;
+  fs.mkdirSync(testCase.dirPath);
+  // long content so it's actually flushed.
+  const content1 = Date.now() + testCase.fileName.repeat(Math.random() * 1e5);


Why Math.random()? I am a bit afraid that this reduces test reproducibility. If you use any random values in a test, please log them to stderr, and if there is an actual reason for Math.random() please add a comment that describes it – but I’d guess just using .repease(10000) would to the job just fine?

Remnant from earlier iteration.
Not needed now.

addaleax

The code changes look good, but it would be good if you could explain why this is being moved to sequential.

Also, I don’t really get why the setInterval() was there before… but well, if it passes CI, fine by me. :)

addaleax · 2017-06-02T19:42:03Z

test/sequential/test-fs-watchfile.js

+  }
+  get dirPath() { return `${common.tmpDir}/${this.dirName}`; }
+  get filePath() { return `${this.dirPath}/${this.fileName}`; }
+}


Aaah, much better. :)

Trott · 2017-06-02T19:52:02Z

I don't think this needs to be moved to sequential. When I stress tested the original in sequential, it still failed on AIX. So I don't think it's necessarily load-dependent. I think just sometimes AIX is unreliable with the directory watchers and it's a known limitation of the OS at this time.

A comment for the setInterval() is probably a good idea. The deal there is that macOS in particular (and maybe other operating systems?) seem to (sometimes?) return an unarmed watcher and so we use the interval to write and write and write again until the watcher notices. It might miss the first one or two. It's a race condition.

refack · 2017-06-02T19:52:50Z

Stress: https://ci.nodejs.org/job/node-stress-single-test/nodes=aix61-ppc64/1267/

The code changes look good, but it would be good if you could explain why this is being moved to sequential.

Also, I don’t really get why the setInterval() was there before… but well, if it passes CI, fine by me. :)

Move to sequential not necessary. Removed.

The backstory is in the refs. tl;dr

in c470f04 we started watching directories to verify we get filename on change
test was flaky on macOS - 3b12a8d (setInterval) solved that
now it's only flaky on AIX
this test is to test if watching a file works even on AIX

Trott · 2017-06-02T19:55:00Z

Oh, also a comment explaining why AIX is included in one but left out of the other would be helpful. Can be as simple as:

// Node.js on AIX can watch reliably on a file but not (at the current time) on a directory.

If you want to be thorough, maybe include a link to one of @gireeshpunathil's comments explaining.

Trott · 2017-06-02T19:57:01Z

Thanks for making this all more thorough.

I wonder if the fs.watch() stuff should all be moved to its own file at this point and leave the rest of the test for fs.watchFile() as the name of the test implies.

refack · 2017-06-02T19:59:26Z

So apparently this is still in progress. My assumption that a big file will replace the need for setInterval and watching a file works on AIX, were not true. So testing variations.

Oh, also a comment explaining why AIX is included in one but left out of the other would be helpful. Can be as simple as:

// Node.js on AIX can watch reliably on a file but not (at the current time) on a directory.
If you want to be thorough, maybe include a link to one of @gireeshpunathil's comments explaining.

I assumed this would land after #13385

I wonder if the fs.watch() stuff should all be moved to its own file at this point and leave the rest of the test for fs.watchFile() as the name of the test implies.

There is a test/sequential/test-fs-watch.js. When this works and stable, I'll move it.

refack · 2017-06-02T21:31:22Z

~~Stress: https://ci.nodejs.org/job/node-stress-single-test/1268/nodes=aix61-ppc64/~~ bad job parameters

refack · 2017-06-02T22:22:24Z

~~Stress: https://ci.nodejs.org/job/node-stress-single-test/1270/nodes=aix61-ppc64/~~

refack · 2017-06-02T23:18:58Z

Stress: https://ci.nodejs.org/job/node-stress-single-test/nodes=aix61-ppc64/1271/

refack · 2017-06-02T23:37:58Z

One more stress: https://ci.nodejs.org/job/node-stress-single-test/1272/nodes=aix61-ppc64/

refack · 2017-06-03T05:11:03Z

AIX remove from "watch dir" case
https://ci.nodejs.org/job/node-stress-single-test/1273/

refack · 2017-06-04T13:55:13Z

Full CI: https://ci.nodejs.org/job/node-test-commit/10350/

refack · 2017-06-04T21:08:45Z

CI: https://ci.nodejs.org/job/node-test-commit/10354/

refack · 2017-06-05T02:17:07Z

new output on macOS:

485	parallel/test-fs-watchfile	
duration_ms	0.294
severity	fail
stack	
watching /Users/iojs/node-tmp/tmp.0/watch1/foo
watching /Users/iojs/node-tmp/tmp.0/watch2
changing /Users/iojs/node-tmp/tmp.0/watch1/foo
changing /Users/iojs/node-tmp/tmp.0/watch2/bar
assert.js:60
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'rename' === 'change'
    at FSWatcher.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-fs-watchfile.js:116:12)
    at FSWatcher.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/common/index.js:517:15)
    at emitTwo (events.js:125:13)
    at FSWatcher.emit (events.js:213:7)
    at FSEvent.FSWatcher._handle.onchange (fs.js:1362:12)

jasnell · 2017-06-05T18:01:26Z

test/parallel/test-fs-watchfile.js

@@ -93,9 +93,10 @@ const cases = [
 ];
 for (const testCase of cases) {
  if (testCase.shouldSkip) continue;
+  console.log(`watching ${testCase[testCase.field]}`)


if the console.log() is not part of the test, please omit it.

Will do when investigation finishes.

refack · 2017-06-05T18:20:28Z

Another full CI: https://ci.nodejs.org/job/node-test-pull-request/8488/
Stress AIX: https://ci.nodejs.org/job/node-stress-single-test/1275/
Stress macOS: https://ci.nodejs.org/job/node-stress-single-test/1276/

refack · 2017-06-05T22:16:26Z

Stress AIX: https://ci.nodejs.org/job/node-stress-single-test/nodes=aix61-ppc64/1277/
CI PR: https://ci.nodejs.org/job/node-test-commit/10380/

refack · 2017-06-06T00:14:42Z

Stress AIX: https://ci.nodejs.org/job/node-stress-single-test/nodes=aix61-ppc64/1278/

refack · 2017-06-06T01:31:00Z

This should be it: https://ci.nodejs.org/job/node-test-pull-request/8500/

refack · 2017-06-06T11:06:03Z

New failures on AIX, so testing with no rebase: https://ci.nodejs.org/job/node-test-commit-aix/6394/

gireeshpunathil · 2017-06-07T04:44:39Z

@refack, I guess the new failures you saw for AIX is in this CI run? if so I believe it is an infra issue, and has resolved by its own in your latest run. Please confirm my understanding, so that there is nothing else to look into from AIX side. Thanks!

refack · 2017-06-07T12:46:00Z

@refack, I guess the new failures you saw for AIX is in this CI run? if so I believe it is an infra issue, and has resolved by its own in your latest run. Please confirm my understanding, so that there is nothing else to look into from AIX side. Thanks!

Yes, https://ci.nodejs.org/job/node-test-commit-aix/6384/nodes=aix61-ppc64/tapResults/
I'm not sure it's infra, so I have this open in one of my tabs for follow up, but...

PR-URL: nodejs#13411 Refs: nodejs#13385 Refs: nodejs#13248 Refs: nodejs#13377 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

refack · 2017-06-07T12:50:42Z

Landed in c05561e

refack · 2017-06-07T12:51:41Z

Extra post-land sanity of master: https://ci.nodejs.org/job/node-test-commit-linuxone/6444/

PR-URL: #13411 Refs: #13385 Refs: #13248 Refs: #13377 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 2, 2017

refack added aix Issues and PRs related to the AIX platform. fs Issues and PRs related to the fs subsystem / file system. labels Jun 2, 2017

addaleax reviewed Jun 2, 2017

View reviewed changes

addaleax approved these changes Jun 2, 2017

View reviewed changes

refack mentioned this pull request Jun 2, 2017

Intermittent failure in parallel/test-fs-watchfile on AIX #13377

Closed

refack force-pushed the fix-13111-13248 branch from 0b91ceb to e39a48b Compare June 2, 2017 19:48

refack added the wip Issues and PRs that are still a work in progress. label Jun 2, 2017

refack force-pushed the fix-13111-13248 branch from e39a48b to 37ceb02 Compare June 2, 2017 22:20

jasnell reviewed Jun 5, 2017

View reviewed changes

refack force-pushed the fix-13111-13248 branch 2 times, most recently from 8845436 to e4d8546 Compare June 5, 2017 22:13

refack changed the title ~~test,fs: test watch of file and directory~~ test,fs: test fs.watch for filename Jun 5, 2017

refack self-assigned this Jun 5, 2017

refack added the macos Issues and PRs related to the macOS platform / OSX. label Jun 5, 2017

jasnell approved these changes Jun 5, 2017

View reviewed changes

test,fs: test fs.watch for filename

c05561e

PR-URL: nodejs#13411 Refs: nodejs#13385 Refs: nodejs#13248 Refs: nodejs#13377 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

refack force-pushed the fix-13111-13248 branch from 0e2c726 to c05561e Compare June 7, 2017 12:49

refack merged commit c05561e into nodejs:master Jun 7, 2017

jasnell pushed a commit that referenced this pull request Jun 7, 2017

test,fs: test fs.watch for filename

658560e

PR-URL: #13411 Refs: #13385 Refs: #13248 Refs: #13377 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

refack deleted the fix-13111-13248 branch June 10, 2017 21:14

refack removed their assignment Jun 12, 2017

refack mentioned this pull request Jun 14, 2017

Investigate flaky test/parallel/test-fs-watch on Raspberry Pi #13642

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Linkgoron mentioned this pull request Apr 3, 2021

investigate flaky test-fs-promises-watch on macOS #37637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test,fs: test fs.watch for `filename` #13411

test,fs: test fs.watch for `filename` #13411

refack commented Jun 2, 2017 •

edited

Loading

refack commented Jun 2, 2017

addaleax Jun 2, 2017

refack Jun 2, 2017 •

edited

Loading

addaleax Jun 2, 2017

refack Jun 2, 2017

addaleax Jun 2, 2017

refack Jun 2, 2017

addaleax left a comment

addaleax Jun 2, 2017

Trott commented Jun 2, 2017

refack commented Jun 2, 2017

Trott commented Jun 2, 2017

Trott commented Jun 2, 2017

refack commented Jun 2, 2017

refack commented Jun 2, 2017 •

edited

Loading

refack commented Jun 2, 2017 •

edited

Loading

refack commented Jun 2, 2017

refack commented Jun 2, 2017

refack commented Jun 3, 2017

refack commented Jun 4, 2017

refack commented Jun 4, 2017

refack commented Jun 5, 2017

jasnell Jun 5, 2017

refack Jun 5, 2017

refack commented Jun 5, 2017

refack commented Jun 5, 2017

refack commented Jun 6, 2017

refack commented Jun 6, 2017

refack commented Jun 6, 2017

gireeshpunathil commented Jun 7, 2017

refack commented Jun 7, 2017

refack commented Jun 7, 2017

refack commented Jun 7, 2017

test,fs: test fs.watch for filename #13411

test,fs: test fs.watch for filename #13411

Conversation

refack commented Jun 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Jun 2, 2017

Choose a reason for hiding this comment

refack Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Jun 2, 2017

refack commented Jun 2, 2017

Trott commented Jun 2, 2017

Trott commented Jun 2, 2017

refack commented Jun 2, 2017

refack commented Jun 2, 2017 • edited Loading

refack commented Jun 2, 2017 • edited Loading

refack commented Jun 2, 2017

refack commented Jun 2, 2017

refack commented Jun 3, 2017

refack commented Jun 4, 2017

refack commented Jun 4, 2017

refack commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 5, 2017

refack commented Jun 5, 2017

refack commented Jun 6, 2017

refack commented Jun 6, 2017

refack commented Jun 6, 2017

gireeshpunathil commented Jun 7, 2017

refack commented Jun 7, 2017

refack commented Jun 7, 2017

refack commented Jun 7, 2017

test,fs: test fs.watch for `filename` #13411

test,fs: test fs.watch for `filename` #13411

refack commented Jun 2, 2017 •

edited

Loading

refack Jun 2, 2017 •

edited

Loading

refack commented Jun 2, 2017 •

edited

Loading

refack commented Jun 2, 2017 •

edited

Loading