test: add tests for fsPromises.chown to increase coverage #20574

shisama · 2018-05-07T12:19:21Z

To increase test coverage for fs/promises, add tests for
methods chown(), filehandle.chown() and lchown().

Checklist

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

BridgeAR · 2018-05-18T01:55:10Z

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

cjihrig · 2018-05-21T11:32:29Z

test/parallel/test-fs-promises.js

@@ -96,6 +98,9 @@ function verifyStatObject(stat) {
    await chmod(dest, 0o666);
    await fchmod(handle, 0o666);

+    await chown(dest, process.getuid(), process.getgid());


process.getuid() and process.getgid() are not defined on Windows.

@cjihrig
Thank you for your review.
Fixed not to call the process.getuid() and process.getgid() on Windows.

Trott

These tests seem to fail just about everywhere in CI...

Trott · 2018-05-22T18:57:14Z

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

because they said we could

Trott · 2018-05-22T19:03:19Z

Rather than skipping on Windows, should we check that we get the right expected error when on Windows?

Trott

Tests still failing on most platforms:

15:19:05 not ok 584 parallel/test-fs-promises
15:19:05   ---
15:19:05   duration_ms: 0.171
15:19:05   severity: fail
15:19:05   exitcode: 1
15:19:05   stack: |-
15:19:05     (node:4855) ExperimentalWarning: The fs.promises API is experimental
15:19:05     /home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-docker/test/common/index.js:798
15:19:05                  (err) => process.nextTick(() => { throw err; }));
15:19:05                                                    ^
15:19:05     
15:19:05     AssertionError [ERR_ASSERTION]: Code: ERR_METHOD_NOT_IMPLEMENTED; The provided arguments length (0) does not match the required ones (1).
15:19:05         at getMessage (internal/errors.js:223:3)
15:19:05         at new NodeError (internal/errors.js:156:13)
15:19:05         at lchown (internal/fs/promises.js:383:11)
15:19:05         at doTest (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-docker/test/parallel/test-fs-promises.js:140:15)
15:19:05   ...

shisama · 2018-05-23T04:20:18Z

@Trott
Thank you for your review.

I missed the doc indicates that the fsPromises.lchown is only implemented on macOS.
Doc: https://nodejs.org/api/fs.html#fs_fspromises_lchown_path_uid_gid

I fixed the method is only called on macOS.

Trott · 2018-05-23T05:47:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15047/ ✔️

jasnell · 2018-05-23T14:40:17Z

Lots of red in the new CI run. Some are flaky failures, some are build bot failures, some are potentially related.... CI seems rather iffy these days... running again: https://ci.nodejs.org/job/node-test-pull-request/15056/

shisama · 2018-05-24T01:46:27Z

@jasnell
I think node-test-commit-linux failure is a flaky failure.
See #20907

Is the failure of node-test-commit-windows-fanned build bot failure?
This is happened on some PullRequest.

jasnell · 2018-05-24T17:52:28Z

yeah, CI in general is in rough shape right now. Let's give it another day or so to see if we can get those issues figured out then give this another run.

mhdawson

LGTM

BridgeAR · 2018-05-29T19:32:22Z

@Trott PTAL. I just checked the CI and added a green check mark next to the CI that you started.

Trott · 2018-05-31T02:04:58Z

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

If CI is green or yellow, feel free to clear my objection (although it looks like this needs a rebase and that will probably mean another CI after the rebase).

Trott · 2018-05-31T02:06:26Z

(And again, I'd prefer that we don't skip the test on unsupported platforms but instead check that we get the expected error. This will help prevent us from making changes that introduce cryptic unhelpful errors by accident. But this is a suggestion and I'm certainly not going to block this on it. It's also something that can be added in a subsequent PR.)

shisama · 2018-06-22T00:33:13Z

@Trott CI failure is that parallel/test-net-bytes-per-incoming-chunk-overhead is timeout. It is related to #21322 ?

Trott · 2018-06-22T03:57:31Z

@Trott CI failure is that parallel/test-net-bytes-per-incoming-chunk-overhead is timeout. It is related to #21322 ?

@shisama Yes, it would seem to be.

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

apapirovski · 2018-06-25T06:41:38Z

@Trott @mhdawson @cjihrig @jasnell @BridgeAR please have another look. In particular @Trott who is currently blocking this PR. Thanks!

cjihrig · 2018-06-25T19:13:15Z

test/parallel/test-fs-promises.js

@@ -133,6 +162,9 @@ function verifyStatObject(stat) {
    if (common.canCreateSymLink()) {
      const newLink = path.resolve(tmpDir, 'baz3.js');
      await symlink(newPath, newLink);
+      if (common.isOSX) {
+        await lchown(newLink, process.getuid(), process.getgid());


If #21498 lands, this could become !common.isWindows.

Fixed it. Thanks.

@cjihrig PTAL Thanks.

ci is green now, clearing my objection

To increase test coverage for fs/promises, add tests for methods chown(), filehandle.chown() and lchown().

Fix not to call `process.getuid()` and `process.getgid()` on Windows.

Add error tests for fsPromises.chown and FileHandle.chown on all platforms.

TimothyGu · 2018-07-30T00:41:34Z

Rebased.

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

TimothyGu · 2018-07-30T06:01:40Z

Windows rerun: https://ci.nodejs.org/job/node-test-commit-windows-fanned/19550/

TimothyGu · 2018-07-30T17:14:23Z

Landed in a4ce449.

To increase test coverage for fs/promises, add tests for methods chown(), filehandle.chown() and lchown(). PR-URL: #20574 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

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

ChALkeR added experimental Issues and PRs related to experimental features. fs Issues and PRs related to the fs subsystem / file system. labels May 8, 2018

BridgeAR approved these changes May 18, 2018

View reviewed changes

BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 18, 2018

jasnell approved these changes May 19, 2018

View reviewed changes

cjihrig reviewed May 21, 2018

View reviewed changes

Trott previously requested changes May 21, 2018

View reviewed changes

shisama force-pushed the test-fs-promises-chown branch from e7b2fdf to b4fd185 Compare May 22, 2018 07:18

cjihrig approved these changes May 22, 2018

View reviewed changes

Trott previously requested changes May 22, 2018

View reviewed changes

shisama changed the title ~~test: add tests for fs/promises chown to increase coverage~~ test: add tests for fsPromises.chown to increase coverage May 23, 2018

mhdawson approved these changes May 28, 2018

View reviewed changes

shisama force-pushed the test-fs-promises-chown branch from 511e744 to 0e3a4ca Compare May 31, 2018 03:36

shisama force-pushed the test-fs-promises-chown branch 2 times, most recently from a41e746 to 8041fa3 Compare June 12, 2018 00:59

shisama force-pushed the test-fs-promises-chown branch from 8041fa3 to 012b2cc Compare June 21, 2018 15:56

cjihrig reviewed Jun 25, 2018

View reviewed changes

shisama force-pushed the test-fs-promises-chown branch from f2d508b to 345ddc5 Compare July 1, 2018 14:12

ChALkeR approved these changes Jul 1, 2018

View reviewed changes

ChALkeR mentioned this pull request Jul 10, 2018

fs/promises test coverage is poor. #20435

Closed

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

shisama added 4 commits July 29, 2018 20:39

test: add tests for fs/promises chown to increase coverage

7521a3e

To increase test coverage for fs/promises, add tests for methods chown(), filehandle.chown() and lchown().

test: fix failure in test-fs-promises on Windows

ed2faa8

Fix not to call `process.getuid()` and `process.getgid()` on Windows.

test: add fsPromises.chown error test

d195aca

Add error tests for fsPromises.chown and FileHandle.chown on all platforms.

test: call fsPromises.lchown() on non-Windows

b1183ac

TimothyGu approved these changes Jul 30, 2018

View reviewed changes

TimothyGu force-pushed the test-fs-promises-chown branch from 345ddc5 to b1183ac Compare July 30, 2018 00:41

TimothyGu self-assigned this Jul 30, 2018

TimothyGu closed this Jul 30, 2018

TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 30, 2018

targos mentioned this pull request Jul 31, 2018

v10.8.0 proposal #22040

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add tests for fsPromises.chown to increase coverage #20574

test: add tests for fsPromises.chown to increase coverage #20574

shisama commented May 7, 2018

BridgeAR commented May 18, 2018

cjihrig May 21, 2018

shisama May 22, 2018

Trott left a comment

Trott commented May 22, 2018 •

edited

Loading

Trott commented May 22, 2018

Trott left a comment •

edited

Loading

shisama commented May 23, 2018

Trott commented May 23, 2018 •

edited by BridgeAR

Loading

jasnell commented May 23, 2018

shisama commented May 24, 2018

jasnell commented May 24, 2018

mhdawson left a comment

BridgeAR commented May 29, 2018

Trott commented May 31, 2018

Trott commented May 31, 2018

shisama commented Jun 22, 2018

Trott commented Jun 22, 2018

apapirovski commented Jun 25, 2018

cjihrig Jun 25, 2018

shisama Jul 1, 2018

shisama Jul 19, 2018

TimothyGu commented Jul 30, 2018

TimothyGu commented Jul 30, 2018

TimothyGu commented Jul 30, 2018

test: add tests for fsPromises.chown to increase coverage #20574

test: add tests for fsPromises.chown to increase coverage #20574

Conversation

shisama commented May 7, 2018

Checklist

BridgeAR commented May 18, 2018

cjihrig May 21, 2018

Choose a reason for hiding this comment

shisama May 22, 2018

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

Trott commented May 22, 2018 • edited Loading

Trott commented May 22, 2018

Trott left a comment • edited Loading

Choose a reason for hiding this comment

shisama commented May 23, 2018

Trott commented May 23, 2018 • edited by BridgeAR Loading

jasnell commented May 23, 2018

shisama commented May 24, 2018

jasnell commented May 24, 2018

mhdawson left a comment

Choose a reason for hiding this comment

BridgeAR commented May 29, 2018

Trott commented May 31, 2018

Trott commented May 31, 2018

shisama commented Jun 22, 2018

Trott commented Jun 22, 2018

apapirovski commented Jun 25, 2018

cjihrig Jun 25, 2018

Choose a reason for hiding this comment

shisama Jul 1, 2018

Choose a reason for hiding this comment

shisama Jul 19, 2018

Choose a reason for hiding this comment

TimothyGu commented Jul 30, 2018

TimothyGu commented Jul 30, 2018

TimothyGu commented Jul 30, 2018

Trott commented May 22, 2018 •

edited

Loading

Trott left a comment •

edited

Loading

Trott commented May 23, 2018 •

edited by BridgeAR

Loading