Skip to content

Conversation

@refack
Copy link
Contributor

@refackrefack 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-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 27, 2017
@refack
Copy link
ContributorAuthor

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

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
ContributorAuthor

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

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
ContributorAuthor

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 (

functionrmdirSync(p,originalEr){
).

@Trott
Copy link
Member

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

functionrmdirSync(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
ContributorAuthor

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
Copy link
ContributorAuthor

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/

@refackrefack self-assigned this May 27, 2017
@refack
Copy link
ContributorAuthor

Found to be unnecessary.

@refackrefack closed this May 30, 2017
@refackrefack deleted the fix-13111-13248 branch May 30, 2017 20:55
@refackrefack restored the fix-13111-13248 branch June 2, 2017 17:51
@refackrefack changed the title test: cleanup after test-fs-watchfile.jstest,fs: test watch of file and directoryJun 2, 2017
@refackrefack removed their assignment Jun 12, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fsIssues and PRs related to the fs subsystem / file system.testIssues 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

@refack@Trott@richardlau@benjamingr@mscdex@nodejs-github-bot