Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented May 8, 2021

@github-actions 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
Trott reviewed May 8, 2021
View reviewed changes
test/parallel/test-fs-write-file.js Outdated
const readOnlyOption = { mode: fs.constants.O_RDONLY, flag: 'r' };
fs.writeFile(filename3, s, readOnlyOption, common.expectsError({
code: 'EBADF',
message: 'EBADF: bad file descriptor, write'
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#38607

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@pd4d10 pd4d10 mentioned this pull request May 9, 2021
Closed
@mhdawson 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
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@mhdawson

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@RaisinTen 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
@RaisinTen
Copy link
Contributor

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

@pd4d10 @RaisinTen
Apply suggestions from code review
a82fb75 
Co-authored-by: Darshan Sen <raisinten@gmail.com>
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@RaisinTen 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
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

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

@jasnell
Copy link
Member

jasnell commented May 21, 2021

Landed in a1f590e

@jasnell jasnell closed this May 21, 2021
jasnell pushed a commit that referenced this pull request May 21, 2021
@pd4d10 @jasnell
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 pd4d10 deleted the patch-test-fs branch May 22, 2021 01:24
danielleadams pushed a commit that referenced this pull request May 31, 2021
@pd4d10 @danielleadams
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 danielleadams mentioned this pull request May 31, 2021
Merged
@joyeecheung joyeecheung mentioned this pull request Jun 1, 2021
Closed
@richardlau
Copy link
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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@pd4d10 @nodejs-github-bot @mhdawson @RaisinTen @jasnell @richardlau @Trott