test, fs: fix chmod mask testing on windows #12835

refack · 2017-05-04T20:08:22Z

Currently this uncovers a regression in fs.fchmodSync(fd, mode_sync);
No solution yet...

Fixes:#12803

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)

fs,libuv,test

vsemozhetbyt · 2017-05-04T20:10:56Z

Refs: #12803

Trott · 2017-05-04T20:14:38Z

test/parallel/test-fs-chmod.js

@@ -139,5 +140,11 @@ if (fs.lchmod) {


 process.on('exit', function() {
+  try {


Is this added because the files being operated on are in the fixtures dir? IMO, the test should be refactored to use the tmp dir instead so that we don't need to do this. Tests shouldn't modify files in fixtures. Then it can check mode on file creation too, I suppose.

Yes. Good idea!

If I copy the files to common.tmpDir it doesn't repro

refack · 2017-05-04T20:16:37Z

/cc @nodejs/fs
CI: https://ci.nodejs.org/job/node-test-commit/9644/

vsemozhetbyt · 2017-05-04T21:48:40Z

Simplified reproducible test for Windows.

refack · 2017-05-04T22:12:01Z

Windows CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/8827/

vsemozhetbyt · 2017-05-04T23:23:43Z

@refack The exhaustive test.

refack · 2017-05-05T00:23:09Z

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

vsemozhetbyt · 2017-05-05T00:24:13Z

test/known_issues/test-fs-fchmod-12835.js

+'use strict';
+const common = require('../common');
+if (common.isWindows) {
+// Windows only: Test fchmod on files with `attrib -a -i`


vsemozhetbyt · 2017-05-05T00:32:35Z

See about known_issues.status file here.

vsemozhetbyt · 2017-05-05T00:34:06Z

test/known_issues/test-fs-fchmod-12835.js

@@ -0,0 +1,34 @@
+'use strict';
+const common = require('../common');
+if (common.isWindows) {


This is not necessary if known_issues.status is properly updated, if I get this right.

I'll need to set it to be excluded for every platform but windows.
This way seems less noisy...

vsemozhetbyt · 2017-05-05T00:37:09Z

test/parallel/test-fs-chmod.js

+const fixture2 = path.join(common.fixturesDir, 'a1.js');
+const file1 = path.join(common.tmpDir, 'a.js');
+const file2 = path.join(common.tmpDir, 'a1.js');
+execSync(`copy ${fixture1} ${file1}`);


/bin/sh: copy: command not found in Linux CI.

refack · 2017-05-05T03:55:47Z

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

refack · 2017-05-05T04:50:11Z

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

vsemozhetbyt · 2017-05-05T13:31:22Z

test/known_issues/test-fs-fchmod-12835.js

+const {execSync} = require('child_process');
+const fs = require('fs');
+
+ok(common.isWindows, "reset of test only for windows")


Linter:

11:22 error Strings must use singlequote quotes 11:55 error Missing semicolon semi

vsemozhetbyt · 2017-05-05T13:32:30Z

test/parallel/test-fs-chmod.js

-const file2 = path.join(common.fixturesDir, 'a1.js');
+const file1 = path.join(common.tmpDir, Date.now() + 'duck.js');
+const file2 = path.join(common.tmpDir, Date.now() + 'goose.js');
+fs.writeFileSync(file1, "ga ga");


Linter: 78:25 error Strings must use singlequote quotes

vsemozhetbyt · 2017-05-05T13:33:02Z

test/parallel/test-fs-chmod.js

+const file1 = path.join(common.tmpDir, Date.now() + 'duck.js');
+const file2 = path.join(common.tmpDir, Date.now() + 'goose.js');
+fs.writeFileSync(file1, "ga ga");
+fs.writeFileSync(file2, "waq waq");


Linter: 79:25 error Strings must use singlequote quotes

vsemozhetbyt · 2017-05-05T13:33:25Z

test/parallel/test-fs-chmod.js

+fs.writeFileSync(file1, "ga ga");
+fs.writeFileSync(file2, "waq waq");
+// to flush some buffers
+console.log('written')


Linter: 81:23 error Missing semicolon semi

refack · 2017-05-05T13:37:09Z

@vsemozhetbyt de-linted.

P.S. how do you run the lint locally, I couldn't find a good command?

sam-github · 2017-05-05T13:38:14Z

@refack make lint?

refack · 2017-05-05T13:39:56Z

@refack make lint?

works so bad on Windows 😭

vsemozhetbyt · 2017-05-05T13:42:17Z

@refack
For all:

node tools/eslint/bin/eslint.js --rulesdir tools/eslint-rules/ --ext=.js,.md benchmark doc lib test tools

or just:

eslint --rulesdir tools/eslint-rules/ --ext=.js,.md benchmark doc lib test tools

For a file:

eslint --rulesdir tools/eslint-rules/ path/to/a/file.js
eslint --rulesdir tools/eslint-rules/ path/to/a/file.md

refack · 2017-05-05T16:40:03Z

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

refack · 2017-05-05T17:32:26Z

@nodejs/platform-macos if this a real bug?

Snippet from test (last line is 94)

const file1 = path.join(common.tmpDir, Date.now() + 'duck.js');
const file2 = path.join(common.tmpDir, Date.now() + 'goose.js');
fs.writeFileSync(file1, 'ga ga');
fs.writeFileSync(file2, 'waq waq');
// to flush some buffers
console.log('written');
console.log('written some more');
if (common.isWindows) {
  execSync(`attrib -a -h -i -r -s ${file1}`);
  execSync(`attrib -a -h -i -r -s ${file2}`);
} else {
  execSync(`touch ${file1}`);
  execSync(`touch ${file2}`);
}

fs.chmod(file1, mode_async.toString(8), common.mustCall((err) => {
  assert.ifError(err);

  console.log(fs.statSync(file1).mode);

Output

364	parallel/test-fs-chmod	
duration_ms	
0.204
severity	
fail
stack	
written
written some more
fs.js:934
  binding.stat(pathModule._makeLong(path));
          ^

Error: ENOENT: no such file or directory, stat '/Users/iojs/node-tmp/tmp.1/1494002742913duck.js'
    at Object.fs.statSync (fs.js:934:11)
    at fs.chmod.common.mustCall (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-fs-chmod.js:94:18)
    at /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/common/index.js:460:15
    at FSReqWrap.oncomplete (fs.js:135:15)

refack · 2017-05-13T03:00:45Z

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

refack · 2017-05-13T03:32:26Z

Failing on freeBSD: https://ci.nodejs.org/job/node-stress-single-test/1213/nodes=freebsd10-64/

refack · 2017-05-13T03:33:31Z

Try to fail on macOS: https://ci.nodejs.org/job/node-stress-single-test/1215/nodes=osx1010/

refack · 2017-05-13T20:17:10Z

Latest CI: https://ci.nodejs.org/job/node-test-commit/9870/
Found an issue on macOS, but couldn't distill it into a known-issue test

refack · 2017-05-15T22:53:42Z

@Trott @vsemozhetbyt PTAL
Current test uncovers two issues

Windows distilled into test-fs-fchmod-12835.js
An issue on macOS with short fileWriteSync test, fs: fix chmod mask testing on windows #12835 (comment)

vsemozhetbyt · 2017-05-15T23:12:59Z

@refack I feel murky on this issue, so I dare not make any definitive review(

Trott · 2017-05-16T03:32:16Z

@nodejs/testing PTAL

Trott · 2017-05-16T03:34:56Z

test/parallel/test-fs-chmod.js

@@ -139,5 +153,7 @@ if (fs.lchmod) {


 process.on('exit', function() {
+  fs.chmodSync(file1, mode_sync);


Why the chmod in the exit handler?

If the file stays RO, common.refreshTmpDir fails

Are you certain about that? If so, it must be Windows-only behavior.

Are you certain about that? If so, it must be Windows-only behavior.

Yes, common.refreshTmpDir fails in a few cases on Windows

OK, well in that case, either surrounding it with if (common.isWindows) and/or providing a comment explaining why it's there might help prevent someone like me from coming along and obliviously removing it in six months. :-D

So let's wait with this. I want to fix common.refreshTmpDir on windows.

refack · 2017-05-16T03:43:36Z

New CI:https://ci.nodejs.org/job/node-test-commit/9914/ (hopefully we got the flakes out of the way)

thefourtheye · 2017-05-16T04:41:45Z

test/known_issues/test-fs-fchmod-12835.js

+const {execSync} = require('child_process');
+const fs = require('fs');
+
+ok(common.isWindows, 'reset of test only for windows');


We generally use common.skip to skip tests.

Since this is a known-issue it's supposed to fail.
So it's either this line or exclude this test for all platforms but Windows in known_issues.status

Otherwise we get:

refael@refaelux:/mnt/d/code/node-cur$ python tools/test.py known_issues === release test-fs-fchmod-12835 [negative] === Path: known_issues/test-fs-fchmod-12835 1..0 # Skipped: undefined Command: out/Release/node /mnt/d/code/node-cur/test/known_issues/test-fs-fchmod-12835.js [00:11|% 100|+ 13|- 1]: Done

Currently this uncovers a regression in `fs.fchmodSync(fd, mode_sync);` No solution yet... Also as issue on macOS in test-fs-chmod:94 fail to access file1 Fixes:nodejs#12803

refack · 2017-05-20T04:27:50Z

Blocking until we fix common.refreshTmpDir on Windows

BridgeAR · 2017-09-20T00:36:34Z

Ping @refack

BridgeAR · 2017-09-23T00:09:37Z

test/parallel/test-fs-chmod.js

@@ -24,9 +24,11 @@ const common = require('../common');
 const assert = require('assert');
 const path = require('path');
 const fs = require('fs');
+const {execSync} = require('child_process');


Is the linter not complaining about this without extra whitespace?^^

It seems the rule was landed later than the last CI here.

BridgeAR · 2017-09-27T22:37:09Z

@refack this needs a rebase

BridgeAR · 2017-11-22T12:38:50Z

Closing due to long inactivity. @refack please reopen if you want to further pursue this.

refack added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label May 4, 2017

refack self-assigned this May 4, 2017

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

refack added fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. labels May 4, 2017

Trott reviewed May 4, 2017

View reviewed changes

refack mentioned this pull request May 4, 2017

fs, test: one set of file attributes in Windows prevents fs.fchmod() from changing mode to RW and breaks incomplete test #12803

Closed

vsemozhetbyt reviewed May 5, 2017

View reviewed changes

refack force-pushed the 12803-chmod-edge branch from e2cdcf0 to c07479e Compare May 5, 2017 13:36

refack force-pushed the 12803-chmod-edge branch from c07479e to c4ce793 Compare May 13, 2017 02:58

refack force-pushed the 12803-chmod-edge branch 3 times, most recently from 6de5aa1 to 1576078 Compare May 13, 2017 20:14

refack added macos Issues and PRs related to the macOS platform / OSX. and removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels May 15, 2017

Trott reviewed May 16, 2017

View reviewed changes

thefourtheye reviewed May 16, 2017

View reviewed changes

refack force-pushed the 12803-chmod-edge branch from 1576078 to 938b8c1 Compare May 16, 2017 19:29

test: fix chmod mask testing on windows

52e3483

Currently this uncovers a regression in `fs.fchmodSync(fd, mode_sync);` No solution yet... Also as issue on macOS in test-fs-chmod:94 fail to access file1 Fixes:nodejs#12803

refack force-pushed the 12803-chmod-edge branch from 938b8c1 to 52e3483 Compare May 19, 2017 19:40

refack added the blocked PRs that are blocked by other issues or PRs. label May 20, 2017

refack mentioned this pull request May 27, 2017

test,fs: test watch of file and directory #13251

Closed

3 tasks

refack removed the blocked PRs that are blocked by other issues or PRs. label Aug 20, 2017

BridgeAR reviewed Sep 24, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

BridgeAR closed this Nov 22, 2017

refack removed their assignment Oct 12, 2018

		@@ -139,5 +140,11 @@ if (fs.lchmod) {


		process.on('exit', function() {
		try {

		@@ -139,5 +153,7 @@ if (fs.lchmod) {


		process.on('exit', function() {
		fs.chmodSync(file1, mode_sync);

test, fs: fix chmod mask testing on windows #12835

test, fs: fix chmod mask testing on windows #12835

Conversation

refack commented May 4, 2017 • edited Loading

Checklist

Affected core subsystem(s)

vsemozhetbyt commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented May 4, 2017

vsemozhetbyt commented May 4, 2017

refack commented May 4, 2017

vsemozhetbyt commented May 4, 2017 • edited Loading

refack commented May 5, 2017

Choose a reason for hiding this comment

vsemozhetbyt commented May 5, 2017

vsemozhetbyt May 5, 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

refack commented May 5, 2017

refack commented May 5, 2017

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

refack commented May 5, 2017

sam-github commented May 5, 2017

refack commented May 5, 2017 • edited Loading

vsemozhetbyt commented May 5, 2017 • edited Loading

refack commented May 5, 2017

refack commented May 5, 2017

Snippet from test (last line is 94)

Output

refack commented May 13, 2017

refack commented May 13, 2017

refack commented May 13, 2017

refack commented May 13, 2017

refack commented May 15, 2017

vsemozhetbyt commented May 15, 2017

Trott commented May 16, 2017

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

Choose a reason for hiding this comment

refack commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented May 20, 2017

BridgeAR commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 27, 2017

BridgeAR commented Nov 22, 2017

refack commented May 4, 2017 •

edited

Loading

vsemozhetbyt commented May 4, 2017 •

edited

Loading

vsemozhetbyt May 5, 2017 •

edited

Loading

refack commented May 5, 2017 •

edited

Loading

vsemozhetbyt commented May 5, 2017 •

edited

Loading