test: add tests for fsPromises.chown to increase coverage by shisama · Pull Request #20574 · nodejs/node · GitHub

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

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

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.

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

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

because they said we could

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

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   ...

@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.

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

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/

@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.

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.

LGTM

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

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).

(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.)

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

@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/

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

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.

Rebased.

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

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

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 <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

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 outstanding review comments, and a CI started. and removed author ready PRs that have at least one approval, no outstanding review comments, 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 outstanding review comments, 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 outstanding review comments, and a CI started. label Jul 30, 2018

targos mentioned this pull request Jul 31, 2018

v10.8.0 proposal #22040

Merged

test: add tests for fsPromises.chown to increase coverage by shisama · Pull Request #20574 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

test: add tests for fsPromises.chown to increase coverage#20574shisama wants to merge 4 commits intonodejs:masternodejs/node:masterfrom shisama:test-fs-promises-chownshisama/node:test-fs-promises-chownCopy head branch name to clipboard

Conversation

shisama commented May 7, 2018

Checklist

Uh oh!

BridgeAR commented May 18, 2018

Uh oh!

cjihrig May 21, 2018

Choose a reason for hiding this comment

Uh oh!

shisama May 22, 2018

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented May 22, 2018

Uh oh!

Trott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shisama commented May 23, 2018

Uh oh!

Trott commented May 23, 2018 • edited by BridgeAR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 23, 2018

Uh oh!

shisama commented May 24, 2018

Uh oh!

jasnell commented May 24, 2018

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented May 29, 2018

Uh oh!

Trott commented May 31, 2018

Uh oh!

Trott commented May 31, 2018

Uh oh!

shisama commented Jun 22, 2018

Uh oh!

Trott commented Jun 22, 2018

Uh oh!

apapirovski commented Jun 25, 2018

Uh oh!

cjihrig Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

shisama Jul 1, 2018

Choose a reason for hiding this comment

Uh oh!

shisama Jul 19, 2018

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Jul 30, 2018

Uh oh!

TimothyGu commented Jul 30, 2018

Uh oh!

TimothyGu commented Jul 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

test: add tests for fsPromises.chown to increase coverage#20574
shisama wants to merge 4 commits into
nodejs:masterfrom
shisama:test-fs-promises-chown

Trott commented May 22, 2018 •
edited
Loading

Trott left a comment •
edited
Loading

Trott commented May 23, 2018 •
edited by BridgeAR
Loading