-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: fix chmod mask testing on windows #12835
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'); | ||
|
||
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! |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the linter not complaining about this without extra whitespace?^^ There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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); | ||
|
@@ -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); | ||
} | ||
|
@@ -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); | ||
} | ||
|
@@ -139,5 +153,7 @@ if (fs.lchmod) { | |
|
||
|
||
process.on('exit', function() { | ||
fs.chmodSync(file1, mode_sync); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the file stays RO, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, well in that case, either surrounding it with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So let's wait with this. I want to fix |
||
fs.chmodSync(file2, mode_sync); | ||
assert.strictEqual(0, openCount); | ||
}); |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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: