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.

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

test: make test-fs-watch-recursive more robust #9228

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 21, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test fs

Description of change

The test was failing on OS X when run in parallel with:

tools/test.py --repeat=99 -J parallel/test-fs-watch-recursive

ENOENT was arising when trying to create a file in the directory created
with fs.mkdtempSync(). I do not know if this is a bug in OS X, a bug
in libuv, or something else. I also do not know this is partially or
completely the cause of the flakiness of the test in CI and locally
(when run as part of make test).

In any event, changing to fs.mkdirSync() resolved the issue.

/cc @nodejs/testing

@Trott Trott added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. macos Issues and PRs related to the macOS platform / OSX. labels Oct 21, 2016
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 21, 2016
The test was failing on OS X when run in parallel with:

```
tools/test.py --repeat=99 -J parallel/test-fs-watch-recursive
```

ENOENT was arising when trying to create a file in the directory created
with `fs.mkdtempSync()`. I do not know if this is a bug in OS X, a bug
in libuv, or something else. I also do not know this is partially or
completely the cause of the flakiness of the test in CI and locally
(when run as part of `make test`).

In any event, changing to `fs.mkdirSync()` resolved the issue.
@Trott Trott force-pushed the fs-watch-recursive branch from 59c6b10 to 199d650 Compare October 21, 2016 20:55
@Trott Trott added the wip Issues and PRs that are still a work in progress. label Oct 21, 2016
@Trott
Copy link
Member Author

Trott commented Oct 21, 2016

@santigimeno has done some looking into this and it looks like this fix is misguided. I've added the in progress label for now. It looks like this is probably a bug in either common.js or test.py, or else a misunderstanding on my part. It looks like test.py -J will run tests in parallel but may not guarantee that they won't share tmp directories? Or maybe a bug in common.js that has a similar-ish result?

@Trott
Copy link
Member Author

Trott commented Oct 21, 2016

I can get the error even with a --repeat value lower than -j such as:

tools/test.py -j 4 --repeat=3 parallel/test-fs-watch-recursive

which yields things like:

$ tools/test.py -j 4 --repeat=3 parallel/test-fs-watch-recursive
=== release test-fs-watch-recursive ===                    
Path: parallel/test-fs-watch-recursive
fs.js:557
  return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode);
                 ^

Error: ENOENT: no such file or directory, open '/Users/trott/io.js/test/tmp.3/yjbVje/watch.txt'
    at Object.fs.openSync (fs.js:557:18)
    at Object.fs.writeFileSync (fs.js:1214:33)
    at Timeout._onTimeout (/Users/trott/io.js/test/parallel/test-fs-watch-recursive.js:39:8)
    at ontimeout (timers.js:365:14)
    at tryOnTimeout (timers.js:237:5)
    at Timer.listOnTimeout (timers.js:207:5)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-fs-watch-recursive.js
[00:00|% 100|+   2|-   1]: Done 
$

It also very occasionally gives me EEXISTS....

@Trott
Copy link
Member Author

Trott commented Oct 22, 2016

Seems to have to do with the interaction of the --repeat parameter with -j in test.py. This fails every time with what seems to be temp directory collisions (two tests trying to use the same tmp directory):

tools/test.py -J --repeat=10 parallel/test-fs-watch-recursive

But this seemingly equivalent command succeeds every time:

tools/test.py -J parallel/test-fs-watch-recursive parallel/test-fs-watch-recursive parallel/test-fs-watch-recursive parallel/test-fs-watch-recursive parallel/test-fs-watch-recursive parallel/test-fs-watch-recursive parallel/test-fs-watch-recursive parallel/test-fs-watch-recursive parallel/test-fs-watch-recursive parallel/test-fs-watch-recursive

/cc @nodejs/testing or maybe @nodejs/build or maybe someone whose Python-knowledgable which I think might be @thefourtheye

@Trott
Copy link
Member Author

Trott commented Oct 23, 2016

The --repeat option was added in ccbb00e so I guess I'll /cc @mhdawson too...

@Trott
Copy link
Member Author

Trott commented Oct 24, 2016

Figured it out. Closing this. PR addressing test.py issue coming momentarily.

@Trott Trott closed this Oct 24, 2016
Trott added a commit to Trott/io.js that referenced this pull request Oct 24, 2016
The repeat option in test.py did not work as expected if `-j` was set to
more than one. Repeated tests running at the same time could share temp
directories and cause test failures. This was observed with:

    tools/test.py -J --repeat=10 parallel/test-fs-watch-recursive

By using copy.deepCopy(), the repeated tests are separate objects and
not references to the same objects. Setting `thread_id` on one of them
will now not change the `thread_id` on all of them. And `thread_id` is
how the temp directory (and common.PORT as well) are determined.

Refs: nodejs#9228
jasnell pushed a commit that referenced this pull request Oct 26, 2016
The repeat option in test.py did not work as expected if `-j` was set to
more than one. Repeated tests running at the same time could share temp
directories and cause test failures. This was observed with:

    tools/test.py -J --repeat=10 parallel/test-fs-watch-recursive

By using copy.deepCopy(), the repeated tests are separate objects and
not references to the same objects. Setting `thread_id` on one of them
will now not change the `thread_id` on all of them. And `thread_id` is
how the temp directory (and common.PORT as well) are determined.

Refs: #9228
PR-URL: #9249
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
The repeat option in test.py did not work as expected if `-j` was set to
more than one. Repeated tests running at the same time could share temp
directories and cause test failures. This was observed with:

    tools/test.py -J --repeat=10 parallel/test-fs-watch-recursive

By using copy.deepCopy(), the repeated tests are separate objects and
not references to the same objects. Setting `thread_id` on one of them
will now not change the `thread_id` on all of them. And `thread_id` is
how the temp directory (and common.PORT as well) are determined.

Refs: #9228
PR-URL: #9249
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
The repeat option in test.py did not work as expected if `-j` was set to
more than one. Repeated tests running at the same time could share temp
directories and cause test failures. This was observed with:

    tools/test.py -J --repeat=10 parallel/test-fs-watch-recursive

By using copy.deepCopy(), the repeated tests are separate objects and
not references to the same objects. Setting `thread_id` on one of them
will now not change the `thread_id` on all of them. And `thread_id` is
how the temp directory (and common.PORT as well) are determined.

Refs: #9228
PR-URL: #9249
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
The repeat option in test.py did not work as expected if `-j` was set to
more than one. Repeated tests running at the same time could share temp
directories and cause test failures. This was observed with:

    tools/test.py -J --repeat=10 parallel/test-fs-watch-recursive

By using copy.deepCopy(), the repeated tests are separate objects and
not references to the same objects. Setting `thread_id` on one of them
will now not change the `thread_id` on all of them. And `thread_id` is
how the temp directory (and common.PORT as well) are determined.

Refs: #9228
PR-URL: #9249
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Trott Trott deleted the fs-watch-recursive branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants