Skip to content

Increase test coverage for fs/promises.js #19811

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
Closed
wants to merge 3 commits into from
Closed

Increase test coverage for fs/promises.js #19811

wants to merge 3 commits into from

Conversation

@humphd
Copy link
Contributor

@humphd humphd commented Apr 4, 2018

I was looking at test coverage for the new things in fs/promises.js and wanted to add some more. This adds test cases using FileHandle objects vs. only using paths or fds.

Due to #19057, I'm unable to run coverage locally on macOS; apologies for not including updated coverage info.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 4, 2018
@Trott
Copy link
Member

Trott commented Apr 5, 2018

Hi @humphd! Welcome and thanks for the PR! It's possible that some/all of this is duplicated in #19605. Might want to compare and remove anything that's duplicated because that one looks like it's going to land very soon. (As in I'm actually in the middle of landing it RIGHT NOW.)

@Trott
Copy link
Member

Trott commented Apr 5, 2018

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

@humphd
Copy link
Contributor Author

humphd commented Apr 5, 2018

@Trott OK, thanks, I'll compare with what's landed there and adjust this.

@Trott
Copy link
Member

Trott commented Apr 5, 2018

(Lone CI failure is unrelated. Can be re-run if this PR isn't going to change, but if there are any changes, we'll have to re-run all of CI anyway. So, either way...)

@humphd humphd force-pushed the improve-fs-promises-coverage branch from 166aa52 to 7a97e40 Compare April 6, 2018 16:37
@humphd
Copy link
Contributor Author

humphd commented Apr 6, 2018

@Trott based on what was added in #19057, I've just reverted my changes to test/parallel/test-fs-promises-writefile.js and just included my additions to test/parallel/test-fs-promises.js, which are unmodified from my previous commit. I've also rebased this.

@Trott Trott force-pushed the improve-fs-promises-coverage branch from 7a97e40 to be70489 Compare April 6, 2018 21:52
@Trott
Copy link
Member

Trott commented Apr 6, 2018

@nodejs/fs @nodejs/testing

@Trott
Copy link
Member

Trott commented Apr 6, 2018

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

BridgeAR
BridgeAR approved these changes Apr 9, 2018
View reviewed changes
test/parallel/test-fs-promises.js Outdated
Copy link
Member

@BridgeAR BridgeAR Apr 9, 2018

Choose a reason for hiding this comment

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

As a general note: we can actually use the following instead:

assert.rejects(
  // `mode` can't be > 0o777
  () => fchmod(handle, (0o777 + 1)),
  {
    code: 'ERR_OUT_OF_RANGE',
    name: 'RangeError [ERR_OUT_OF_RANGE]'
  }
);

test/parallel/test-fs-promises.js Outdated
Copy link
Member

@BridgeAR BridgeAR Apr 9, 2018

Choose a reason for hiding this comment

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

Nit: can you please remove the console.log here?

thefourtheye
thefourtheye reviewed Apr 9, 2018
View reviewed changes
test/parallel/test-fs-promises.js Outdated
Copy link
Contributor

@thefourtheye thefourtheye Apr 9, 2018

Choose a reason for hiding this comment

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

Shouldn't this be compared with buf2?

Copy link
Contributor

@thefourtheye thefourtheye Apr 9, 2018

Choose a reason for hiding this comment

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

Nit: Also, if we reduce magic numbers (like 11 in this case) it would be better.

thefourtheye
thefourtheye reviewed Apr 9, 2018
View reviewed changes
test/parallel/test-fs-promises.js Outdated
Copy link
Contributor

@thefourtheye thefourtheye Apr 9, 2018

Choose a reason for hiding this comment

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

It would be better, if did this for chmod as well.

@BridgeAR
Copy link
Member

BridgeAR commented Apr 28, 2018

@humphd would you be so kind and have a look at the comments? :-)

@humphd
Copy link
Contributor Author

humphd commented Apr 28, 2018

@BridgeAR apologies for the delay. I'll update this week.

@ChALkeR ChALkeR mentioned this pull request Apr 29, 2018
Merged
4 tasks
@ChALkeR ChALkeR mentioned this pull request Apr 30, 2018
Closed
@humphd humphd force-pushed the improve-fs-promises-coverage branch from be70489 to d20bde9 Compare April 30, 2018 19:42
@humphd
Copy link
Contributor Author

humphd commented Apr 30, 2018

OK, I've rebased and done the following based on the reviews above:

  • switched try/catchs I added to use assert.rejects instead
  • removed stray console.log
  • gotten rid of magic numbers
  • corrected comparison of buf vs. buf2
  • added test cases for (0o777 + 1) on chmod and fchmod as well. I hope I've interpreted what was requested above correctly with this one. Please let me know if I should alter this.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 30, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Apr 30, 2018

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

TimothyGu
TimothyGu reviewed Apr 30, 2018
View reviewed changes
test/parallel/test-fs-promises.js Outdated

await chmod(dest, 0o666);
await fchmod(handle, 0o666);
handle.chmod(0o666);
Copy link
Member

@TimothyGu TimothyGu Apr 30, 2018

Choose a reason for hiding this comment

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

Should this be awaited?

Copy link
Contributor Author

@humphd humphd Apr 30, 2018

Choose a reason for hiding this comment

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

Indeed, thank you for spotting this. I'll fix.

@addaleax
Copy link
Member

addaleax commented May 5, 2018

New CI: https://ci.nodejs.org/job/node-test-commit/18242/

mhdawson
mhdawson approved these changes May 7, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@ChALkeR ChALkeR added the experimental Issues and PRs related to experimental features. label May 8, 2018
@mhdawson
Copy link
Member

mhdawson commented May 10, 2018 •
edited by addaleax
Loading

OSX CI failure looks unrelated, opened #20660

New CI run: https://ci.nodejs.org/job/node-test-pull-request/14796/

@addaleax
Copy link
Member

addaleax commented May 14, 2018 •
edited
Loading

Landed in fcc46ee 🎉

@addaleax addaleax closed this May 14, 2018
addaleax pushed a commit that referenced this pull request May 14, 2018
@humphd @addaleax
test: increase test coverage for fs/promises.js
fcc46ee 
PR-URL: #19811
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax pushed a commit that referenced this pull request May 14, 2018
@humphd @addaleax
test: increase test coverage for fs/promises.js
2b6e8cc 
PR-URL: #19811
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@addaleax addaleax mentioned this pull request May 14, 2018
Merged
@humphd
Copy link
Contributor Author

humphd commented May 15, 2018

Thanks for landing this @addaleax, and to the rest for your reviews and help spotting mistakes.

@joyeecheung joyeecheung mentioned this pull request May 17, 2018
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@mhdawson mhdawson mhdawson approved these changes

+2 more reviewers

@thefourtheye thefourtheye thefourtheye left review comments

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. experimental Issues and PRs related to experimental features. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@humphd @Trott @BridgeAR @addaleax @mhdawson @jasnell @thefourtheye @TimothyGu @ChALkeR @nodejs-github-bot