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.

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

Increase test coverage for fs/promises.js #19811

Closed
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

@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
Copy link
Member

Trott commented Apr 6, 2018

@nodejs/fs @nodejs/testing

@Trott
Copy link
Member

Trott commented Apr 6, 2018

code: 'ERR_OUT_OF_RANGE',
type: RangeError
})(err);
}
Copy link
Member

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

await mkdtemp(1);
} catch (err) {
// mkdtemp() expects to get a string prefix.
console.log('err', err);
Copy link
Member

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?

await handle.write(buf2);
const ret2 = await handle.read(Buffer.alloc(11), 0, 11, 0);
assert.strictEqual(ret2.bytesRead, 11);
assert.deepStrictEqual(ret2.buffer, buf);
Copy link
Contributor

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

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.

await chmod(dest, 0o666);
await fchmod(handle, 0o666);
handle.chmod(0o666);
try {
await fchmod(handle, (0o777 + 1));
Copy link
Contributor

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

@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
4 tasks
@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

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

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

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

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

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

Landed in fcc46ee 🎉

@addaleax addaleax closed this May 14, 2018
addaleax pushed a commit that referenced this pull request May 14, 2018
PR-URL: #19811
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax pushed a commit that referenced this pull request May 14, 2018
PR-URL: #19811
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@addaleax addaleax mentioned this pull request May 14, 2018
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

10 participants