test: add test cases for fsPromises #19064

watilde · 2018-02-28T20:13:45Z

Add tests of lchmod, chown, fchown and lchown. These tests will increase the coverage of fs/promises: https://coverage.nodejs.org/coverage-3f78d3fcf8029a0a/root/fs/promises.js.html.

Checklist

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

Affected core subsystem(s)

test

watilde · 2018-02-28T20:17:09Z

ci: https://ci.nodejs.org/job/node-test-pull-request/13412/

cjihrig · 2018-02-28T20:24:22Z

test/parallel/test-fs-promises.js

@@ -35,6 +39,8 @@ const {
 } = fsPromises;

 const tmpDir = tmpdir.path;
+const gid = process.getgid();


These functions are not available on Windows.

oops... I will fix it.

Ok, I added !common.isWindows.

benjamingr

Approving given green CI (and the windows fix)

watilde · 2018-03-01T12:40:58Z

new ci: https://ci.nodejs.org/job/node-test-pull-request/13422/
~~https://ci.nodejs.org/job/node-test-pull-request/13420/~~

I didn't know the following backgrounds:

Add tests of lchmod, chown, fchown and lchown.

cjihrig

LGTM, although some basic assertions that the function calls are actually working as intended would be nice.

watilde · 2018-03-02T22:04:27Z

landed in c05c73a

Add tests of lchmod, chown, fchown and lchown. PR-URL: #19064 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Trott · 2018-03-03T00:02:12Z

Unfortunately, this appears to be flaky on Raspberry Pi in CI: https://ci.nodejs.org/job/node-test-binary-arm/13951/RUN_SUBSET=1,label=pi3-raspbian-jessie/console

18:17:41 not ok 79 parallel/test-fs-promises
18:17:41   ---
18:17:41   duration_ms: 1.23
18:17:41   severity: fail
18:17:41   stack: |-
18:17:41     (node:1568) ExperimentalWarning: The fs/promises API is experimental
18:17:41     /home/iojs/build/workspace/node-test-binary-arm/test/common/index.js:766
18:17:41                  (err) => process.nextTick(() => { throw err; }));
18:17:41                                                    ^
18:17:41     
18:17:41     Error: EPERM: operation not permitted, chown '/home/iojs/build/workspace/node-test-binary-arm/test/.tmp.0/baz.js'

Can we revert until it's sorted out?

Trott · 2018-03-03T00:03:09Z

It looks like it failed in the CI run for this PR: https://ci.nodejs.org/job/node-test-binary-arm/13910/RUN_SUBSET=2,label=pi3-raspbian-jessie/console

I think it may be failing 100%, not merely flaky/unreliable.

This reverts commit c05c73a. The commit breaks Raspberry Pi CI. Refs: nodejs#19064 (comment)

Trott · 2018-03-03T00:07:55Z

PR to revert: #19101

If fixing it is simple/easy, then I'd be happy to see a fix land instead.

One thing that might be relevant is that I think the Pi devices have their temp directories mounted over NFS. So that may be a cause of fs oddities not seen on other devices. Although it is strange that it's only showing up on one of the three Pi variants, so I'm not really sure what's going on (but also didn't look very close TBH).

/cc @rvagg @nodejs/build

watilde · 2018-03-03T00:12:10Z

Wow... I'm sorry for my commit and thank you for the revert PR Rich. I also will investigate the logs.

Trott · 2018-03-03T00:15:36Z

Wow... I'm sorry for my commit and thank you for the revert PR Rich. I also will investigate the logs.

No problem. These things tend to happen when CI is permanently red already. :-( Definitely have to get that fixed....

This reverts commit c05c73a. The commit breaks Raspberry Pi CI. PR-URL: nodejs#19101 Ref: nodejs#19064 (comment) Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

joyeecheung · 2018-03-03T08:04:29Z

BTW I am also seeing the same error on Windows 10 + VS2017

joyeecheung · 2018-03-03T08:05:37Z

Ah, never mind, I am seeing errors from symlink, which is not touched by this PR

rvagg · 2018-03-04T11:38:04Z

Yeah, that's it @Trott, can't chown on the nfs mount even for the same uid/gid. The problem I think is that the current config is all_squash from the server and forces a particular uid and gid so the nfs client has no control over ownership. This doesn't have to be the case, I could opt for a more sophisticated idmapd thing. We had that running at one stage but it's really hard to get right so for this new NFS server I opted for the simple option.

I'm loath to try and figure it out right now cause once I start messing with NFS it's a big investment to follow it through to completion and I don't have such a large block of time atm. Could we just add a armv6 & armv7 exclusion in the tests for now for this? The idea behind expanding coverage to this functionality is good, it's just our infra getting in the way.

Trott · 2018-03-04T21:24:53Z

@rvagg I think the ideal solution is to fix the test to accommodate this situation rather than alter the setup on the Raspberry Pi devices. I have to imagine existing fs tests work around this issue somehow already.

Add tests of lchmod, chown, fchown and lchown. PR-URL: nodejs#19064 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This reverts commit c05c73a. The commit breaks Raspberry Pi CI. PR-URL: nodejs#19101 Ref: nodejs#19064 (comment) Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 28, 2018

cjihrig reviewed Feb 28, 2018

View reviewed changes

benjamingr approved these changes Mar 1, 2018

View reviewed changes

watilde force-pushed the test/fs-promise branch from 58ca32b to d593cc2 Compare March 1, 2018 12:39

test: add test cases for fsPromises

f3890a3

Add tests of lchmod, chown, fchown and lchown.

watilde force-pushed the test/fs-promise branch from d593cc2 to f3890a3 Compare March 1, 2018 13:08

jasnell approved these changes Mar 1, 2018

View reviewed changes

cjihrig approved these changes Mar 1, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2018

BridgeAR approved these changes Mar 1, 2018

View reviewed changes

watilde closed this Mar 2, 2018

watilde deleted the test/fs-promise branch March 2, 2018 22:04

Trott added a commit to Trott/io.js that referenced this pull request Mar 3, 2018

Revert "test: add test cases for fsPromises"

fd8e920

This reverts commit c05c73a. The commit breaks Raspberry Pi CI. Refs: nodejs#19064 (comment)

Trott mentioned this pull request Mar 3, 2018

Revert "test: add test cases for fsPromises" #19101

Closed

2 tasks

addaleax added dont-land-on-v6.x labels Mar 5, 2018

addaleax added dont-land-on-v9.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add test cases for fsPromises #19064

test: add test cases for fsPromises #19064

watilde commented Feb 28, 2018 •

edited

Loading

watilde commented Feb 28, 2018

cjihrig Feb 28, 2018

watilde Feb 28, 2018

watilde Mar 1, 2018

benjamingr left a comment

watilde commented Mar 1, 2018 •

edited

Loading

cjihrig left a comment

watilde commented Mar 2, 2018

Trott commented Mar 3, 2018

Trott commented Mar 3, 2018 •

edited

Loading

Trott commented Mar 3, 2018

watilde commented Mar 3, 2018

Trott commented Mar 3, 2018

joyeecheung commented Mar 3, 2018

joyeecheung commented Mar 3, 2018

rvagg commented Mar 4, 2018

Trott commented Mar 4, 2018

test: add test cases for fsPromises #19064

test: add test cases for fsPromises #19064

Conversation

watilde commented Feb 28, 2018 • edited Loading

Checklist

Affected core subsystem(s)

watilde commented Feb 28, 2018

cjihrig Feb 28, 2018

Choose a reason for hiding this comment

watilde Feb 28, 2018

Choose a reason for hiding this comment

watilde Mar 1, 2018

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

watilde commented Mar 1, 2018 • edited Loading

cjihrig left a comment

Choose a reason for hiding this comment

watilde commented Mar 2, 2018

Trott commented Mar 3, 2018

Trott commented Mar 3, 2018 • edited Loading

Trott commented Mar 3, 2018

watilde commented Mar 3, 2018

Trott commented Mar 3, 2018

joyeecheung commented Mar 3, 2018

joyeecheung commented Mar 3, 2018

rvagg commented Mar 4, 2018

Trott commented Mar 4, 2018

watilde commented Feb 28, 2018 •

edited

Loading

watilde commented Mar 1, 2018 •

edited

Loading

Trott commented Mar 3, 2018 •

edited

Loading