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

Increase test coverage for fs/promises.js by humphd · Pull Request #19811 · nodejs/node · GitHub

/ node Public

Increase test coverage for fs/promises.js - #19811

Closed
humphd wants to merge 3 commits into
nodejs:masterfrom
humphd:improve-fs-promises-coverage
ConversationCommits (3)ChecksFiles changed
Closed

Increase test coverage for fs/promises.js#19811
humphd wants to merge 3 commits into
nodejs:masterfrom
humphd:improve-fs-promises-coverage

Conversation

humphd commented Apr 4, 2018

Copy link
Copy Markdown
Contributor

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 added the test Issues and PRs related to the tests. label Apr 4, 2018

Trott commented Apr 5, 2018

Copy link
Copy Markdown
Member

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 commented Apr 5, 2018

Copy link
Copy Markdown
Member

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

humphd commented Apr 5, 2018

Copy link
Copy Markdown
Contributor Author

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

Trott commented Apr 5, 2018

Copy link
Copy Markdown
Member

(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 force-pushed the improve-fs-promises-coverage branch from 166aa52 to 7a97e40 Compare April 6, 2018 16:37

humphd commented Apr 6, 2018

Copy link
Copy Markdown
Contributor Author

@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 force-pushed the improve-fs-promises-coverage branch from 7a97e40 to be70489 Compare April 6, 2018 21:52

Trott commented Apr 6, 2018

Copy link
Copy Markdown
Member

@nodejs/fs @nodejs/testing

Trott commented Apr 6, 2018

Copy link
Copy Markdown
Member

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

BridgeAR approved these changes Apr 9, 2018
View reviewed changes
Comment thread test/parallel/test-fs-promises.js Outdated

BridgeAR Apr 9, 2018

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

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]'
  }
);

Comment thread test/parallel/test-fs-promises.js Outdated

BridgeAR Apr 9, 2018

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

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

thefourtheye reviewed Apr 9, 2018
View reviewed changes
Comment thread test/parallel/test-fs-promises.js Outdated

thefourtheye Apr 9, 2018

Copy link
Copy Markdown
Contributor

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

Shouldn't this be compared with buf2?

thefourtheye Apr 9, 2018

Copy link
Copy Markdown
Contributor

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

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

thefourtheye reviewed Apr 9, 2018
View reviewed changes
Comment thread test/parallel/test-fs-promises.js Outdated

thefourtheye Apr 9, 2018

Copy link
Copy Markdown
Contributor

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

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

BridgeAR commented Apr 28, 2018

Copy link
Copy Markdown
Member

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

humphd commented Apr 28, 2018

Copy link
Copy Markdown
Contributor Author

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

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

humphd commented Apr 30, 2018

Copy link
Copy Markdown
Contributor Author

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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 30, 2018

BridgeAR commented Apr 30, 2018

Copy link
Copy Markdown
Member

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

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

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

TimothyGu Apr 30, 2018

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

Should this be awaited?

humphd Apr 30, 2018

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

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

addaleax commented May 5, 2018

Copy link
Copy Markdown
Member

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

mhdawson approved these changes May 7, 2018
View reviewed changes

mhdawson left a comment

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

LGTM

ChALkeR added the experimental Issues and PRs related to experimental features. label May 8, 2018

mhdawson commented May 10, 2018 •
edited by addaleax
Loading

Copy link
Copy Markdown
Member

OSX CI failure looks unrelated, opened #20660

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

addaleax commented May 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Landed in fcc46ee 🎉

addaleax closed this May 14, 2018
addaleax pushed a commit that referenced this pull request May 14, 2018
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
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 mentioned this pull request May 14, 2018
Merged

humphd commented May 15, 2018

Copy link
Copy Markdown
Contributor Author

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

joyeecheung mentioned this pull request May 17, 2018
Closed
4 tasks
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

jasnell jasnell approved these changes
BridgeAR BridgeAR approved these changes
+3 more reviewers
thefourtheye thefourtheye left review comments
TimothyGu TimothyGu approved these changes
mhdawson mhdawson 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 outstanding review comments, 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

Back | FazBrowse Home | New Git URL