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,fs: test watch of file and directory #13251

Closed
wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented May 27, 2017

cleanup before and after test-fs-watchfile
Maybe fix: #13248
Maybe fix: #13377

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 27, 2017
@refack refack force-pushed the fix-13111-13248 branch from f91dd5a to d2430cb Compare May 27, 2017 03:14
@refack
Copy link
Contributor Author

refack commented May 27, 2017

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label May 27, 2017
@Trott
Copy link
Member

Trott commented May 27, 2017

OS X stress test failed. (You will probably want to cancel it so it doesn't keep that host from being used for other test runs.)

I'm pretty sure this issue isn't about removing files but is instead about an OS-specific race condition in file watching. See #13248 (comment)

@refack
Copy link
Contributor Author

refack commented May 27, 2017

I'm pretty sure this issue isn't about removing files but is instead about an OS-specific race condition in file watching. See #13248 (comment)

Ack. Stopped. But this PR is good to have anyway, no?

@Trott
Copy link
Member

Trott commented May 27, 2017

But this PR is good to have anyway, no?

We don't generally have tests clean up after themselves. I suppose exceptions can be made for tests that generate absurdly large files or that mess with file permissions. But generally, we don't do it because it's the responsibility of the test using tmpDir to clean it up before using it.

If there's some sort of subtlety with file watchers that makes cleaning up the file a Good Thing, then sure. I'm not aware of that, though, so you'll have to point me in the right direction to get me on board. :-D

@refack
Copy link
Contributor Author

refack commented May 27, 2017

If there's some sort of subtlety with file watchers that makes cleaning up the file a Good Thing, then sure. I'm not aware of that, though, so you'll have to point me in the right direction to get me on board. :-D

On windows when there is a subdirectory in the tmpDir running common.refreshTmpDir(); throws.

Error: ENOTEMPTY: directory not empty, rmdir 'D:\code\node-cur\test\tmp'
    at Object.fs.rmdirSync (fs.js:856:18)
    at rmdirSync (D:\code\node-cur\test\common\index.js:152:10)
    at rimrafSync (D:\code\node-cur\test\common\index.js:122:7)
    at Object.exports.refreshTmpDir (D:\code\node-cur\test\common\index.js:158:3)
    at Object.<anonymous> (D:\code\node-cur\test\parallel\test-fs-watchfile.js:39:8)

@richardlau
Copy link
Member

On windows when there is a subdirectory in the tmpDir running common.refreshTmpDir(); throws.

Odd, common.rmdirSync() catches ENOTEMPTY and attempts to common.rimrafSync() the contents (

function rmdirSync(p, originalEr) {
).

@Trott
Copy link
Member

Trott commented May 27, 2017

Odd, common.rmdirSync() catches ENOTEMPTY and attempts to common.rimrafSync() the contents (

function rmdirSync(p, originalEr) {
).

And I've never seen this Windows issue on CI. And no one has ever reported it except @refack as far as I know. I'm sure it's happening, but I suspect the cause is something specific and/or peculiar...

@refack
Copy link
Contributor Author

refack commented May 27, 2017

And I've never seen this Windows issue on CI. And no one has ever reported it except @refack as far as I know. I'm sure it's happening, but I suspect the cause is something specific and/or peculiar...

Ya'll are right. I put a trace on, and found It's a local process (TortoiseGit) pre-checking the status of files in the working dir, that keeps some files locked.
I want to say I've seen it in the CI but it might have been a different case (RO files in #12835)

@refack refack force-pushed the fix-13111-13248 branch from d2430cb to 6053fa1 Compare May 27, 2017 13:24
@refack
Copy link
Contributor Author

refack commented May 27, 2017

Removed calls to unlink/rmdir but IMHO refreshTmpDir should stay, or merged with @Trott's #13252
CI: https://ci.nodejs.org/job/node-test-commit/10187/
CI: https://ci.nodejs.org/job/node-test-commit/10188/

@refack refack force-pushed the fix-13111-13248 branch from 6053fa1 to 35ae48c Compare May 27, 2017 13:29
@refack refack self-assigned this May 27, 2017
@refack
Copy link
Contributor Author

refack commented May 30, 2017

Found to be unnecessary.

@refack refack closed this May 30, 2017
@refack refack deleted the fix-13111-13248 branch May 30, 2017 20:55
@refack refack restored the fix-13111-13248 branch June 2, 2017 17:51
@refack refack changed the title test: cleanup after test-fs-watchfile.js test,fs: test watch of file and directory Jun 2, 2017
@refack refack removed their assignment Jun 12, 2017
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intermittent failure in parallel/test-fs-watchfile on AIX test-fs-watchfile failure on OSX - intermittent ?
6 participants