test: improve coverage of lib/fs.js by pd4d10 · Pull Request #38604 · nodejs/node

pd4d10 · 2021-05-08T18:37:45Z

Refs:
https://coverage.nodejs.org/coverage-29f1b609ba5d12d3/lib/fs.js.html#L2045
https://coverage.nodejs.org/coverage-29f1b609ba5d12d3/lib/fs.js.html#L2056

Trott · 2021-05-08T20:27:34Z

test/parallel/test-fs-write-file.js

+  const readOnlyOption = { mode: fs.constants.O_RDONLY, flag: 'r' };
+  fs.writeFile(filename3, s, readOnlyOption, common.expectsError({
+    code: 'EBADF',
+    message: 'EBADF: bad file descriptor, write'


Not a change to make in this PR, but I wonder if that error message could be improved. Unless I'm misunderstanding, the problem from the user perspective is mode/permissions and not that the file descriptor is bad.

Agreed. Since writeFile(fd) and writeFile(path) share the same util function writeAll, this change may need a refactor.

I'll open an issue to track it and look into it later.

According to https://man7.org/linux/man-pages/man2/write.2.html:

EBADF: fd is not a valid file descriptor or is not open for writing.

EPERM: The operation was prevented by a file seal; see fcntl(2).

Considering that the file has been opened in the read-only mode and the program is attempting to write to it, isn't EBADF more appropriate here?

I guess the point is that it's a writeFile operation in the user perspective, but the current error is about file descriptor, which may causes confusing.

This function just reflects the error that is returned from uv_fs_write, which again reflects the error the OS returns. I don't think we replace the error codes for the other functions, so doing it for this particular function feels a little odd, don't you think?

@pd4d10 This test is failing for Windows. Could you please update it to expect an EPERM for Windows?

test/parallel/test-fs-write-file.js

test/parallel/test-fs-writefile-with-fd.js

RaisinTen · 2021-05-18T14:48:02Z

Removing the author-ready for now as there is a failure on Windows. Will add it back in after it is fixed.

test/parallel/test-fs-writefile-with-fd.js

test/parallel/test-fs-write-file.js

Co-authored-by: Darshan Sen <raisinten@gmail.com>

nodejs-github-bot · 2021-05-21T21:21:24Z

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

jasnell · 2021-05-21T22:45:01Z

Landed in a1f590e

PR-URL: #38604 Refs: https://coverage.nodejs.org/coverage-29f1b609ba5d12d3/lib/fs.js.html#L2045 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

richardlau · 2021-07-16T13:34:13Z

This lands cleanly on v14.x-staging but the modified test fails

=== release test-fs-writefile-with-fd ===
Path: parallel/test-fs-writefile-with-fd
/home/rlau/sandbox/github/trees/v14.x-staging/test/parallel/test-fs-writefile-with-fd.js:74
  const controller = new AbortController();
                     ^

ReferenceError: AbortController is not defined
    at Object.<anonymous> (/home/rlau/sandbox/github/trees/v14.x-staging/test/parallel/test-fs-writefile-with-fd.js:74:22)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12)

I tried adding

// Flags: --experimental-abortcontroller

at the beginning of the test file but still get an error:

$ node test/parallel/test-fs-writefile-with-fd.js
NOTE: The test started as a child_process using these flags: [ '--experimental-abortcontroller' ] Use NODE_SKIP_FLAG_CHECK to run the test with the original flags.
(node:553301) ExperimentalWarning: AbortController is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
/home/rlau/sandbox/github/trees/v14.x-staging/test/common/index.js:577
    assert.strictEqual(descriptor.enumerable, false);
                                  ^

TypeError: Cannot read property 'enumerable' of undefined
    at /home/rlau/sandbox/github/trees/v14.x-staging/test/common/index.js:577:35
    at /home/rlau/sandbox/github/trees/v14.x-staging/test/common/index.js:376:15
    at writeAll (fs.js:1449:7)
    at Object.writeFile (fs.js:1494:5)
    at /home/rlau/sandbox/github/trees/v14.x-staging/test/parallel/test-fs-writefile-with-fd.js:80:8
    at /home/rlau/sandbox/github/trees/v14.x-staging/test/common/index.js:341:17
    at /home/rlau/sandbox/github/trees/v14.x-staging/test/common/index.js:376:15
    at FSReqCallback.oncomplete (fs.js:180:23)

github-actions bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 8, 2021

Trott reviewed May 8, 2021

View reviewed changes

Trott approved these changes May 8, 2021

View reviewed changes

pd4d10 mentioned this pull request May 9, 2021

Correct the error type of fs.writeFile with read-only mode #38607

Closed

pd4d10 force-pushed the patch-test-fs branch from 7805163 to 8413f0f Compare May 9, 2021 02:40

RaisinTen reviewed May 9, 2021

View reviewed changes

test/parallel/test-fs-write-file.js Outdated Show resolved Hide resolved

pd4d10 force-pushed the patch-test-fs branch from 8413f0f to edd2d47 Compare May 9, 2021 15:58

jasnell approved these changes May 10, 2021

View reviewed changes

RaisinTen approved these changes May 10, 2021

View reviewed changes

mhdawson added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 14, 2021

This comment has been minimized.

Sign in to view

RaisinTen reviewed May 18, 2021

View reviewed changes

test/parallel/test-fs-writefile-with-fd.js Outdated Show resolved Hide resolved

RaisinTen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 18, 2021

test: improve coverage of lib/fs.js

1478960

pd4d10 force-pushed the patch-test-fs branch from edd2d47 to 1478960 Compare May 18, 2021 15:13

RaisinTen reviewed May 19, 2021

View reviewed changes

test/parallel/test-fs-writefile-with-fd.js Outdated Show resolved Hide resolved

RaisinTen reviewed May 19, 2021

View reviewed changes

test/parallel/test-fs-write-file.js Outdated Show resolved Hide resolved

Apply suggestions from code review

a82fb75

Co-authored-by: Darshan Sen <raisinten@gmail.com>

This comment has been minimized.

Sign in to view

RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 20, 2021

This comment has been minimized.

Sign in to view

jasnell closed this May 21, 2021

pd4d10 deleted the patch-test-fs branch May 22, 2021 01:24

danielleadams mentioned this pull request May 31, 2021

v16.3.0 proposal #38874

Merged

joyeecheung mentioned this pull request Jun 1, 2021

test: close fds properly in tests #38884

Closed

richardlau added the backport-requested-v14.x label Jul 16, 2021

Uh oh!

Conversation

pd4d10 commented May 8, 2021

Uh oh!

Trott May 8, 2021

Choose a reason for hiding this comment

Uh oh!

pd4d10 May 9, 2021

Choose a reason for hiding this comment

Uh oh!

pd4d10 May 9, 2021

Choose a reason for hiding this comment

Uh oh!

RaisinTen May 9, 2021

Choose a reason for hiding this comment

Uh oh!

pd4d10 May 9, 2021

Choose a reason for hiding this comment

Uh oh!

RaisinTen May 10, 2021

Choose a reason for hiding this comment

Uh oh!

RaisinTen May 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

RaisinTen commented May 18, 2021

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 21, 2021 • edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 21, 2021

Uh oh!

richardlau commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nodejs-github-bot commented May 21, 2021 •

edited by jasnell

Loading