★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/20574Nouvelle recherche | Portail wanayoo
Skip to content

/node

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: add tests for fsPromises.chown to increase coverage #20574

Closed
wants to merge 4 commits into from

Conversation

Reviewers

@cjihrig cjihrig

@ChALkeR ChALkeR

@jasnell jasnell

@TimothyGu TimothyGu

@BridgeAR BridgeAR

@mhdawson mhdawson

@Trott Trott

Assignees

@TimothyGu TimothyGu

Labels
experimental fs test
Projects
None yet
Milestone
No milestone
10 participants
@shisama @BridgeAR @Trott @jasnell @apapirovski @TimothyGu @ChALkeR @cjihrig @mhdawson @nodejs-github-bot
@shisama
Copy link
Contributor

shisama commented May 7, 2018

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

@nodejs-github-bot nodejs-github-bot added the test label May 7, 2018

@ChALkeR ChALkeR added experimental fs labels May 8, 2018

@BridgeAR

BridgeAR approved these changes May 18, 2018
View changes

@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

BridgeAR commented May 18, 2018

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

@BridgeAR BridgeAR added author ready and removed author ready labels May 18, 2018

@jasnell

jasnell approved these changes May 19, 2018
View changes

@cjihrig

cjihrig reviewed May 21, 2018
View changes

test/parallel/test-fs-promises.js Outdated
@@ -96,6 +98,9 @@ function verifyStatObject(stat) {
await chmod(dest, 0o666);
await fchmod(handle, 0o666);

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

This comment has been minimized.

Sign in to view
Copy link
@cjihrig

cjihrig May 21, 2018

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@shisama

shisama May 22, 2018

Author Contributor

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

Trott reviewed May 21, 2018
View changes

Copy link
Member

Trott left a comment

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

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

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented May 22, 2018 •
edited

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

cjihrig approved these changes May 22, 2018
View changes

@cjihrig cjihrig dismissed Trott’s stale review May 22, 2018

because they said we could

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented May 22, 2018

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

Trott reviewed May 22, 2018
View changes

Copy link
Member

Trott left a comment •
edited

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

@shisama

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

shisama commented May 23, 2018

@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

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented May 23, 2018 •
edited by BridgeAR

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

This comment has been minimized.

Sign in to view
Copy link
Member

jasnell commented May 23, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

shisama commented May 24, 2018

@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

This comment has been minimized.

Sign in to view
Copy link
Member

jasnell commented May 24, 2018

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

mhdawson approved these changes May 28, 2018
View changes

Copy link
Member

mhdawson left a comment

LGTM
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

BridgeAR commented May 29, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented May 31, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented May 31, 2018

(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 shisama force-pushed the shisama:test-fs-promises-chown branch from 511e744 to 0e3a4ca May 31, 2018

@shisama shisama force-pushed the shisama:test-fs-promises-chown branch 2 times, most recently from a41e746 to 8041fa3 Jun 11, 2018

@shisama shisama force-pushed the shisama:test-fs-promises-chown branch from 8041fa3 to 012b2cc Jun 21, 2018

@shisama

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

shisama commented Jun 22, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Jun 22, 2018

@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

This comment has been minimized.

Sign in to view
Copy link
Member

apapirovski commented Jun 25, 2018

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

cjihrig reviewed Jun 25, 2018
View changes

test/parallel/test-fs-promises.js Outdated
@@ -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());

This comment has been minimized.

Sign in to view
Copy link
@cjihrig

cjihrig Jun 25, 2018

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@shisama

shisama Jul 1, 2018

Author Contributor

Fixed it. Thanks.

This comment has been minimized.

Sign in to view
Copy link
@shisama

shisama Jul 19, 2018

Author Contributor

@cjihrig PTAL Thanks.

@Trott Trott dismissed their stale review Jun 26, 2018

ci is green now, clearing my objection

@shisama shisama force-pushed the shisama:test-fs-promises-chown branch from f2d508b to 345ddc5 Jul 1, 2018

@ChALkeR

ChALkeR approved these changes Jul 1, 2018
View changes

@ChALkeR ChALkeR referenced this pull request Jul 10, 2018

Closed

fs/promises test coverage is poor. #20435

@TimothyGu TimothyGu added the author ready label Jul 30, 2018

shisama added some commits May 7, 2018

@shisama @TimothyGu
test: add tests for fs/promises chown to increase coverage 
To increase test coverage for fs/promises, add tests for
methods chown(), filehandle.chown() and lchown().
7521a3e
@shisama @TimothyGu
test: fix failure in test-fs-promises on Windows 
Fix not to call `process.getuid()` and `process.getgid()` on Windows.
ed2faa8
@shisama @TimothyGu
test: add fsPromises.chown error test 
Add error tests for fsPromises.chown and FileHandle.chown on all
platforms.
d195aca
@shisama @TimothyGu
test: call fsPromises.lchown() on non-Windows
Loading status checks…
b1183ac
@TimothyGu

TimothyGu approved these changes Jul 30, 2018
View changes

@TimothyGu TimothyGu force-pushed the shisama:test-fs-promises-chown branch from 345ddc5 to b1183ac Jul 30, 2018

@TimothyGu

This comment has been minimized.

Sign in to view
Copy link
Member

TimothyGu commented Jul 30, 2018

Rebased.

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

@TimothyGu TimothyGu self-assigned this Jul 30, 2018

@TimothyGu

This comment has been minimized.

Sign in to view
Copy link
Member

TimothyGu commented Jul 30, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

TimothyGu commented Jul 30, 2018

Landed in a4ce449.

@TimothyGu TimothyGu closed this Jul 30, 2018

@TimothyGu TimothyGu removed the author ready label Jul 30, 2018

TimothyGu added a commit that referenced this pull request Jul 30, 2018

@shisama @TimothyGu
test: add tests for fs/promises chown functions 
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>
Loading status checks…
a4ce449

targos added a commit that referenced this pull request Jul 31, 2018

@shisama @targos
test: add tests for fs/promises chown functions 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
4510ca3

@targos targos referenced this pull request Jul 31, 2018

Merged

v10.8.0 proposal #22040

This was referenced Aug 1, 2018

Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.