Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions test/known_issues/test-fs-fchmod-12835.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
'use strict';
const common = require('../common');

// Windows only: Test fchmod on files with `attrib -a -h -i -r -s`
// setting RO then can't set RW back

const {strictEqual, ok} = require('assert');
const {execSync} = require('child_process');
const fs = require('fs');

ok(common.isWindows, 'reset of test only for windows');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally use common.skip to skip tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


process.on('exit', function() {
console.log('Trying to cleanup');
try {
execSync(`del /q /f ${tmpFile}`);
} catch (e) {
console.log(e);
}
});

const RO = 0o100444; // 33060 0b1000000100100100
const RW = 0o100666; // 33206 0b1000000110110110

const tmpFile = `${common.tmpDir}\\${Date.now()}gigi.js`;

const fd = fs.openSync(tmpFile, 'a');
execSync(`attrib -a -h -i -r -s ${tmpFile}`);
fs.fchmodSync(fd, RO);
const actual2 = fs.fstatSync(fd).mode;
console.log(`${tmpFile} mode: ${actual2}`);
strictEqual(actual2, RO);

fs.fchmodSync(fd, RW); // This does not work.
const actual3 = fs.fstatSync(fd).mode;
console.log(`${tmpFile} mode: ${actual3}`);
strictEqual(actual3, RW); // BANG!
4 changes: 4 additions & 0 deletions test/parallel/parallel.status
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ prefix parallel
[true] # This section applies to all platforms

[$system==win32]
# https://github.com/nodejs/node/issues/12803
test-fs-chmod : PASS,FLAKY

[$system==linux]

[$system==macos]
# https://github.com/nodejs/node/issues/12803
test-fs-chmod : PASS,FLAKY

[$arch==arm || $arch==arm64]

Expand Down
28 changes: 22 additions & 6 deletions test/parallel/test-fs-chmod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


let mode_async;
let mode_sync;
const windows_writable_mask = 0o222;

// Need to hijack fs.open/close to make sure that things
// get closed once they're opened.
Expand Down Expand Up @@ -64,15 +66,27 @@ function closeSync() {

// On Windows chmod is only able to manipulate read-only bit
if (common.isWindows) {
mode_async = 0o400; // read-only
mode_sync = 0o600; // read-write
mode_async = 0o444; // read-only
mode_sync = 0o666; // read-write
} else {
mode_async = 0o777;
mode_sync = 0o644;
}

const file1 = path.join(common.fixturesDir, 'a.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');
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);
Expand All @@ -87,7 +101,7 @@ fs.chmod(file1, mode_async.toString(8), common.mustCall((err) => {

fs.chmodSync(file1, mode_sync);
if (common.isWindows) {
assert.ok((fs.statSync(file1).mode & 0o777) & mode_sync);
assert.ok((fs.statSync(file1).mode & 0o777) & windows_writable_mask);
} else {
assert.strictEqual(mode_sync, fs.statSync(file1).mode & 0o777);
}
Expand All @@ -109,7 +123,7 @@ fs.open(file2, 'a', common.mustCall((err, fd) => {

fs.fchmodSync(fd, mode_sync);
if (common.isWindows) {
assert.ok((fs.fstatSync(fd).mode & 0o777) & mode_sync);
assert.ok((fs.fstatSync(fd).mode & 0o777) & windows_writable_mask);
} else {
assert.strictEqual(mode_sync, fs.fstatSync(fd).mode & 0o777);
}
Expand Down Expand Up @@ -139,5 +153,7 @@ if (fs.lchmod) {


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the chmod in the exit handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the file stays RO, common.refreshTmpDir fails

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fs.chmodSync(file2, mode_sync);
assert.strictEqual(0, openCount);
});