test: check that modules backed by read-only files can load (win32) #20116

billti · 2018-04-18T04:14:28Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

This is the test-case for the issue at #20112 . This fails with the issue present, and passes with the fix applied.

Note: Even before any changes, vcbuild test gave a ton of failures for me, so not sure if that is my machine or something more fundamental. This test behaves as expected though.

CC @vsemozhetbyt, @cjihrig, @richardlau

Trott · 2018-04-18T04:22:43Z

test/parallel/test-module-readonly.js

+
+if (process.platform == 'win32') {
+    // Create readOnlyMod.js and set to read only
+    const readOnlyMod = fixtures.path('readOnlyMod');


Instead of writing to the fixtures dir, and thus the source tree, can we create this in tmpdir instead?

I think the sum total of the change would be:

require tmpdir instead of fixtures

call tmpdir.refresh() once

Use tmpdir.path instead of fixtures.path

Trott · 2018-04-18T04:23:22Z

test/parallel/test-module-readonly.js

+    cp.execSync(`icacls.exe "${readOnlyModFullPath}" /remove "%USERNAME%" /inheritance:e`);
+
+    // Delete the file
+    fs.unlinkSync(readOnlyModFullPath);


(If we use the tmpdir module instead of the fixtures module, then there is no need to unlink.)

I just pushed a change to use tmpdir. I figured cleaning up is safer than having another run try and overwrite the same file (permissions shouldn't conflict as it should be a different tmpdir if under a different account, but just to be safe...).

(If you use common/tmpdir rather than os.tmpdir, there is no need to clean up because tests are expected to refresh tmpdir before they do anything in it.)

Trott · 2018-04-18T04:25:02Z

Hi, @billti! Thanks for this!

@nodejs/testing PTAL

Trott · 2018-04-18T04:25:20Z

I guess @nodejs/platform-windows too...

Trott · 2018-04-18T05:14:36Z

Whoops! Not os.tmpdir(). Use common/tmpdir! https://github.com/nodejs/node/tree/master/test/common#tmpdir-module

Or look at another test to see some example code:

node/test/parallel/test-fs-write-stream-close-without-callback.js

Lines 7 to 10 in bf6ce47

    
           const tmpdir = require('../common/tmpdir'); 
        
           tmpdir.refresh(); 
        
           const s = fs.createWriteStream(path.join(tmpdir.path, 'nocallback'));

Trott · 2018-04-18T05:24:28Z

Hmmm...I think this pull request is against the wrong branch. It should be against master and it will flow from there into v10.x-staging and v10.x...

billti · 2018-04-18T05:28:31Z

I switched to the tmpdir requested.

I added the test for the v10.x branch as from the linked issue I thought that was what is being targeted ASAP in order to get the 10.0 release done. I can target whatever branch you want though. Just let me know.

vsemozhetbyt · 2018-04-18T08:15:39Z

Thank you!
There are some linting nits in the test, You can quickly lint your file in this way from the project root folder:

node tools/node_modules/eslint/bin/eslint.js test/parallel/test-module-readonly.js

Currently, you will get:

Some errors:

j:\temp\_git\node-fork\test\parallel\1test-module-readonly.js
   1:1   error  Mandatory module "common" must be loaded       node-core/required-modules
   1:1   error  Use the global form of 'use strict'            strict
   8:22  error  Expected '===' and instead saw '=='            eqeqeq
   9:1   error  Expected indentation of 2 spaces but found 4   indent
  10:1   error  Expected indentation of 2 spaces but found 4   indent
  11:1   error  Expected indentation of 2 spaces but found 4   indent
  12:1   error  Expected indentation of 2 spaces but found 4   indent
  12:47  error  Strings must use singlequote                   quotes
  13:1   error  Expected indentation of 2 spaces but found 4   indent
  13:43  error  Strings must use singlequote                   quotes
  15:1   error  Expected indentation of 2 spaces but found 4   indent
  15:1   error  Line 15 exceeds the maximum line length of 80  max-len
  16:1   error  Expected indentation of 2 spaces but found 4   indent
  16:1   error  Line 16 exceeds the maximum line length of 80  max-len
  17:1   error  Expected indentation of 2 spaces but found 4   indent
  18:1   error  Expected indentation of 2 spaces but found 4   indent
  20:1   error  Expected indentation of 2 spaces but found 4   indent
  21:1   error  Expected indentation of 2 spaces but found 4   indent
  22:1   error  Expected indentation of 4 spaces but found 8   indent
  23:1   error  Expected indentation of 4 spaces but found 8   indent
  23:15  error  'mymod' is assigned a value but never used     no-unused-vars
  24:1   error  Expected indentation of 2 spaces but found 4   indent
  24:7   error  Expected space(s) after "catch"                keyword-spacing
  25:1   error  Expected indentation of 4 spaces but found 8   indent
  26:1   error  Expected indentation of 2 spaces but found 4   indent
  28:1   error  Expected indentation of 2 spaces but found 4   indent
  29:1   error  Expected indentation of 2 spaces but found 4   indent
  29:1   error  Line 29 exceeds the maximum line length of 80  max-len
  31:1   error  Expected indentation of 2 spaces but found 4   indent
  32:1   error  Expected indentation of 2 spaces but found 4   indent
  34:1   error  Expected indentation of 2 spaces but found 4   indent
  34:17  error  Use assert.ifError(except) instead             node-core/prefer-assert-iferror

✖ 32 problems (32 errors, 0 warnings)
  24 errors, 0 warnings potentially fixable with the `--fix` option.

About Mandatory module "common" must be loaded you can read in https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md

bnoordhuis · 2018-04-18T09:13:55Z

test/parallel/test-module-readonly.js

+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+
+if (process.platform == 'win32') {


common.isWindows

bnoordhuis · 2018-04-18T09:14:33Z

test/parallel/test-module-readonly.js

+    fs.writeFileSync(readOnlyModFullPath, "module.exports = 42;");
+
+    // Removed any inherited ACEs, and any explicitly granted ACEs for the current user
+    cp.execSync(`icacls.exe "${readOnlyModFullPath}" /inheritance:r /remove "%USERNAME%"`);


Long line here, whitespace/indent errors elsewhere. Can you run make lint?

As this is a Windows test the equivalent is vcbuild lint (or vcbuild lint-js to just lint the JavaScript files).

richardlau

Thanks for this. We have a guide to writing tests at https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md which explains things like starting the test with require('../common').

richardlau · 2018-04-18T12:38:10Z

test/parallel/test-module-readonly.js

+    fs.writeFileSync(readOnlyModFullPath, "module.exports = 42;");
+
+    // Removed any inherited ACEs, and any explicitly granted ACEs for the current user
+    cp.execSync(`icacls.exe "${readOnlyModFullPath}" /inheritance:r /remove "%USERNAME%"`);


As this is a Windows test the equivalent is vcbuild lint (or vcbuild lint-js to just lint the JavaScript files).

richardlau

Oh and as has been pointed out by @Trott, please retarget this to the master branch as that's where the libuv fix will go first before being backported into the v10.x branches.

richardlau · 2018-04-18T12:48:44Z

Adding the blocked label as this test is expected to fail (even when the lint issues have been addressed) until we get a libuv with libuv/libuv#1800.

cjihrig · 2018-04-18T12:49:57Z

test/parallel/test-module-readonly.js

+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+
+if (process.platform == 'win32') {


Since this is currently a Windows only test, you can do:

if (!common.isWindows) { common.skip('test only runs on Windows'); }

This will skip the test on other platforms and exit the process.

cjihrig · 2018-04-18T12:50:58Z

test/parallel/test-module-readonly.js

+    // Delete the file
+    fs.unlinkSync(readOnlyModFullPath);
+
+    if (except) throw except;


Instead of an if, we'd ideally want an assertion, right?

e.g., assert.ifError(except) https://nodejs.org/dist/latest-v9.x/docs/api/assert.html#assert_assert_iferror_value

Trott · 2018-04-18T17:14:53Z

vcbuild lint will flag a bunch of stuff on this file (or at least should) like needing a 'use strict'; and requiring the test common module. But I think those will be easy fixes.

Trott · 2018-04-18T17:16:24Z

(Sorry for all the style nits that are getting picked. We try to put as many of them into the linter as possible rather than having them come up in PR review. Thanks for your patience with it all!)

billti · 2018-04-18T18:29:07Z

No problem. I think I have all the issues addressed. Just trying to run all the tests/linting now. I'll open a new pull request against 'master' when it's ready, and link & close this one when done.

billti · 2018-04-18T19:01:49Z

Opened new pull request against master in #20138 which should address all comments (so far 😉).

Check that modules backed by read-only files can load (win32)

d3b071a

nodejs-github-bot added test Issues and PRs related to the tests. v10.x labels Apr 18, 2018

Trott reviewed Apr 18, 2018

View reviewed changes

test: switched to using tmpdir for temporary module

1d21f3a

billti mentioned this pull request Apr 18, 2018

Node 10, npm gives an EPERM on every operation #20112

Closed

test: use the test defined tmpdir

860578d

vsemozhetbyt added fs Issues and PRs related to the fs subsystem / file system. and removed fs Issues and PRs related to the fs subsystem / file system. labels Apr 18, 2018

bnoordhuis reviewed Apr 18, 2018

View reviewed changes

richardlau requested changes Apr 18, 2018

View reviewed changes

richardlau added the blocked PRs that are blocked by other issues or PRs. label Apr 18, 2018

cjihrig reviewed Apr 18, 2018

View reviewed changes

billti closed this Apr 18, 2018

billti deleted the test-read-only-modules branch April 18, 2018 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: check that modules backed by read-only files can load (win32) #20116

test: check that modules backed by read-only files can load (win32) #20116

billti commented Apr 18, 2018

Trott Apr 18, 2018

Trott Apr 18, 2018 •

edited

Loading

Trott Apr 18, 2018

billti Apr 18, 2018

Trott Apr 18, 2018 •

edited

Loading

Trott commented Apr 18, 2018

Trott commented Apr 18, 2018

Trott commented Apr 18, 2018 •

edited

Loading

Trott commented Apr 18, 2018

billti commented Apr 18, 2018

vsemozhetbyt commented Apr 18, 2018 •

edited

Loading

bnoordhuis Apr 18, 2018

bnoordhuis Apr 18, 2018

richardlau Apr 18, 2018

richardlau left a comment

richardlau Apr 18, 2018

richardlau left a comment

richardlau commented Apr 18, 2018

cjihrig Apr 18, 2018

cjihrig Apr 18, 2018

richardlau Apr 18, 2018

Trott commented Apr 18, 2018

Trott commented Apr 18, 2018

billti commented Apr 18, 2018

billti commented Apr 18, 2018

test: check that modules backed by read-only files can load (win32) #20116

test: check that modules backed by read-only files can load (win32) #20116

Conversation

billti commented Apr 18, 2018

Checklist

Choose a reason for hiding this comment

Trott Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Trott commented Apr 18, 2018

Trott commented Apr 18, 2018

Trott commented Apr 18, 2018 • edited Loading

Trott commented Apr 18, 2018

billti commented Apr 18, 2018

vsemozhetbyt commented Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlau left a comment

Choose a reason for hiding this comment

richardlau commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Apr 18, 2018

Trott commented Apr 18, 2018

billti commented Apr 18, 2018

billti commented Apr 18, 2018

Trott Apr 18, 2018 •

edited

Loading

Trott Apr 18, 2018 •

edited

Loading

Trott commented Apr 18, 2018 •

edited

Loading

vsemozhetbyt commented Apr 18, 2018 •

edited

Loading