FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

test: improve coverage of lib/fs.js - #38604

Closed
pd4d10 wants to merge 2 commits into
nodejs:masterfrom
pd4d10:patch-test-fs
ConversationCommits (2)ChecksFiles changed
Closed

test: improve coverage of lib/fs.js#38604
pd4d10 wants to merge 2 commits into
nodejs:masterfrom
pd4d10:patch-test-fs

Conversation

pd4d10 commented May 8, 2021

Copy link
Copy Markdown
Contributor

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
Comment thread test/parallel/test-fs-write-file.js Outdated

Trott May 8, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

pd4d10 May 9, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

pd4d10 May 9, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

#38607

RaisinTen May 9, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

pd4d10 May 9, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

RaisinTen May 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

RaisinTen May 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

pd4d10 mentioned this pull request May 9, 2021
Closed
RaisinTen reviewed May 9, 2021
View reviewed changes
Comment thread test/parallel/test-fs-write-file.js Outdated
mhdawson added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 14, 2021

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
RaisinTen reviewed May 18, 2021
View reviewed changes
Comment thread test/parallel/test-fs-writefile-with-fd.js Outdated
RaisinTen removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 18, 2021

RaisinTen commented May 18, 2021

Copy link
Copy Markdown
Member

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

RaisinTen reviewed May 19, 2021
View reviewed changes
Comment thread test/parallel/test-fs-writefile-with-fd.js Outdated
RaisinTen reviewed May 19, 2021
View reviewed changes
Comment thread test/parallel/test-fs-write-file.js Outdated
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 outstanding review comments, and a CI started. label May 20, 2021

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

nodejs-github-bot commented May 21, 2021 •
edited by jasnell
Loading

Copy link
Copy Markdown
Collaborator

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

jasnell commented May 21, 2021

Copy link
Copy Markdown
Member

Landed in a1f590e

jasnell closed this May 21, 2021
jasnell pushed a commit that referenced this pull request May 21, 2021
test: improve coverage of lib/fs.js
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>
pd4d10 deleted the patch-test-fs branch May 22, 2021 01:24
danielleadams pushed a commit that referenced this pull request May 31, 2021
test: improve coverage of lib/fs.js
695e982 
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>
danielleadams mentioned this pull request May 31, 2021
Merged
joyeecheung mentioned this pull request Jun 1, 2021
Closed

richardlau commented Jul 16, 2021

Copy link
Copy Markdown
Member

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)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

RaisinTen RaisinTen approved these changes
jasnell jasnell approved these changes
Trott Trott approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL