Skip to content
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: set umask explicitly #25213

Closed
wants to merge 1 commit into from

Conversation

@i8-pi
Copy link
Contributor

commented Dec 25, 2018

Some tests which create files and check file permissions assume the
umask is compatible with 022, and break when set to something like 007.
Explicitly set umask to 022

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

I'm seeing test errors

=== release test-fs-mkdir-mode-mask ===                                       
Path: parallel/test-fs-mkdir-mode-mask
assert.js:86
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

416 !== 420

    at test (/home/i8-pi/src/node/test/parallel/test-fs-mkdir-mode-mask.js:32:12)
    at Object.<anonymous> (/home/i8-pi/src/node/test/parallel/test-fs-mkdir-mode-mask.js:44:1)

when running
umask 007; make -j4 test

test: set umask explicitly
Some tests which create files and check file permissions assume the
umask is compatible with 022, and break when set to something like 007.
Explicitly set umask to 022
@Trott

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

The issue here is that the temp directory is being created with more restrictive permissions than that being used by the test. Rather than fixing in the Python test runner, might it be better to change the tmpdir module (in test/common) to specify mode 755 when creating the temp directory? This would have the following advantages:

  • Fixes it both with and without the Python test runner. (./node test/parallel/test-fs-mkdir-mode-mask.js will work no matter what your starting umask.)

  • Keeps all the tmpdir code in one place.

  • Allows us to run tests with different umasks easily to make sure they still work. In other words, Node.js should work as expected no matter what the starting umask, but by forcing the starting umask, maybe we miss something.

Counterarguments I can think of would be that this solution is simpler, and there may be edge cases where it's more robust too although I can't think of any.

@i8-pi

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2018

Sorry, I didn't list all failing tests. Here are the rest:

=== release test-fs-open-mode-mask ===                               
Path: parallel/test-fs-open-mode-mask
assert.js:86
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

416 !== 420

    at test (/home/i8-pi/src/node/test/parallel/test-fs-open-mode-mask.js:25:12)
    at Object.<anonymous> (/home/i8-pi/src/node/test/parallel/test-fs-open-mode-mask.js:41:1)
=== release test-fs-promises-file-handle-chmod ===                         
Path: parallel/test-fs-promises-file-handle-chmod
(node:9455) ExperimentalWarning: The fs.promises API is experimental
/home/i8-pi/src/node/test/common/index.js:690
const crashOnUnhandledRejection = (err) => { throw err; };
                                             ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:

288 !== 292

    at validateFilePermission (/home/i8-pi/src/node/test/parallel/test-fs-promises-file-handle-chmod.js:22:10)

so this isn't purely a tmpdir issue

I would consider the umask part of the test environment that the test framework should sanitize before each run, just like with environment variables. A common entry point to all test would be the best place to set it, if the Python test runner is not such a place

If there are tests that should be run with a different umask, I think they should be separate tests where we explicitly override the default, so a single test run covers all cases

@Trott

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

so this isn't purely a tmpdir issue

All of the listed failures are indeed the same tmpdir issue (which makes sense because tests generally shouldn't be creating files outside of tmpdir).

I would consider the umask part of the test environment that the test framework should sanitize before each run, just like with environment variables. A common entry point to all test would be the best place to set it, if the Python test runner is not such a place

If there are tests that should be run with a different umask, I think they should be separate tests where we explicitly override the default, so a single test run covers all cases

That all makes a lot of sense to me. Perhaps both things should be done.

@Trott
Trott approved these changes Dec 26, 2018
@Trott

This comment has been minimized.

@Trott Trott added the author ready label Dec 26, 2018

@Trott

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

Belated welcome @i8-pi and thanks for the pull request! As you might guess, reviews can be a bit slow to happen this time of year, even for simple changes, so thanks in advance for your patience.

Trott added a commit to Trott/io.js that referenced this pull request Dec 26, 2018
test: set umask for tests
nodejs#25213 proposes setting umask in the
Python test runner to avoid spurious test failures when running from a
shell with a restrictive umask. This is a good idea, but will only fix
the issue for tests run with the Python runner. Set it in
`common/index.js` as well so that it fixes it even when tests are run
directly with a `node` binary, bypassing the Python test runner.
@Trott Trott referenced this pull request Dec 26, 2018
1 of 2 tasks complete
@bnoordhuis
Copy link
Member

left a comment

@addaleax

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

@addaleax

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

Landed in ee592c3

@addaleax addaleax closed this Jan 2, 2019

addaleax added a commit that referenced this pull request Jan 2, 2019
test: set umask explicitly
Some tests which create files and check file permissions assume the
umask is compatible with 022, and break when set to something like 007.
Explicitly set umask to 022

PR-URL: #25213
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax added a commit that referenced this pull request Jan 5, 2019
test: set umask explicitly
Some tests which create files and check file permissions assume the
umask is compatible with 022, and break when set to something like 007.
Explicitly set umask to 022

PR-URL: #25213
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Trott added a commit to Trott/io.js that referenced this pull request Jan 9, 2019
test: set umask for tests
nodejs#25213 proposes setting umask in the
Python test runner to avoid spurious test failures when running from a
shell with a restrictive umask. This is a good idea, but will only fix
the issue for tests run with the Python runner. Set it in
`common/index.js` as well so that it fixes it even when tests are run
directly with a `node` binary, bypassing the Python test runner.
Trott added a commit to Trott/io.js that referenced this pull request Jan 10, 2019
test: set umask for tests
nodejs#25213 proposes setting umask in the
Python test runner to avoid spurious test failures when running from a
shell with a restrictive umask. This is a good idea, but will only fix
the issue for tests run with the Python runner. Set it in
`common/index.js` as well so that it fixes it even when tests are run
directly with a `node` binary, bypassing the Python test runner.

PR-URL: nodejs#25229
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
test: set umask explicitly
Some tests which create files and check file permissions assume the
umask is compatible with 022, and break when set to something like 007.
Explicitly set umask to 022

PR-URL: nodejs#25213
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax added a commit that referenced this pull request Jan 14, 2019
test: set umask for tests
#25213 proposes setting umask in the
Python test runner to avoid spurious test failures when running from a
shell with a restrictive umask. This is a good idea, but will only fix
the issue for tests run with the Python runner. Set it in
`common/index.js` as well so that it fixes it even when tests are run
directly with a `node` binary, bypassing the Python test runner.

PR-URL: #25229
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
test: set umask explicitly
Some tests which create files and check file permissions assume the
umask is compatible with 022, and break when set to something like 007.
Explicitly set umask to 022

PR-URL: nodejs#25213
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
test: set umask for tests
nodejs#25213 proposes setting umask in the
Python test runner to avoid spurious test failures when running from a
shell with a restrictive umask. This is a good idea, but will only fix
the issue for tests run with the Python runner. Set it in
`common/index.js` as well so that it fixes it even when tests are run
directly with a `node` binary, bypassing the Python test runner.

PR-URL: nodejs#25229
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
BethGriggs added a commit that referenced this pull request Apr 28, 2019
test: set umask explicitly
Some tests which create files and check file permissions assume the
umask is compatible with 022, and break when set to something like 007.
Explicitly set umask to 022

PR-URL: #25213
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BethGriggs BethGriggs referenced this pull request May 1, 2019
BethGriggs added a commit that referenced this pull request May 10, 2019
test: set umask explicitly
Some tests which create files and check file permissions assume the
umask is compatible with 022, and break when set to something like 007.
Explicitly set umask to 022

PR-URL: #25213
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins added a commit that referenced this pull request May 16, 2019
test: set umask explicitly
Some tests which create files and check file permissions assume the
umask is compatible with 022, and break when set to something like 007.
Explicitly set umask to 022

PR-URL: #25213
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.