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

test: add cb error test for fs.close method #29970

Closed
wants to merge 3 commits into from

Conversation

@teorossi82
Copy link
Contributor

teorossi82 commented Oct 14, 2019

Added test on fs.close method when callback is not a function.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 left a comment

Please call fs.closeSync(fd) at the bottom of the test file.

(Or, better, put the test case in a callback, and then have the close be async too. If you do that, make sure you wrap the callbacks in common.mustCall().)

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 14, 2019

I wouldn't mind these new lines being block-scoped (add a { before the new block and a } after) so that fd doesn't leak out to other test cases added later.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 15, 2019

Out of curiosity, what caused you to write this test? Were you reviewing coverage.nodejs.org and this test is intended to add coverage where we currently don't have coverage? Or is there a different reason you're doing this?

@Trott
Trott approved these changes Oct 15, 2019
Copy link
Member

Trott left a comment

LGTM with a closeSync() added after the forEach stuff.

const errObj = {
code: 'ERR_INVALID_CALLBACK',
name: 'TypeError'
};

This comment has been minimized.

Copy link
@Trott

Trott Oct 15, 2019

Member

Minor nit-pick: You can move the errObj definition outside of the forEach() to below the fd definition. This way, it's only being defined once.

@teorossi82

This comment has been minimized.

Copy link
Contributor Author

teorossi82 commented Oct 15, 2019

Out of curiosity, what caused you to write this test?

I have just took a look to coverage.nodejs.org page and found that this piece of code was not covered. I would like to help you reach 100% coverage :-)

@teorossi82 teorossi82 requested a review from Fishrock123 Oct 15, 2019
@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 15, 2019

CI (probably) won't pass until #29979 lands first. If a Collaborator wants to be the second fast-track approval on that PR, thumbs up at #29979 (comment).

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott Trott added the author ready label Oct 16, 2019
@teorossi82

This comment has been minimized.

Copy link
Contributor Author

teorossi82 commented Oct 16, 2019

@Trott I have to do something to unlock problems with CI or it's not related to my changes and I just have to wait?

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 16, 2019

@Trott I have to do something to unlock problems with CI or it's not related to my changes and I just have to wait?

CI is passing. The GitHub interface doesn't always update. Not sure if that's a problem with our bot or with GitHub or something else. But this is all good to go. You don't need to do anything but wait for someone to land it. Which I'm going to do right now!

Trott added a commit that referenced this pull request Oct 16, 2019
Provides some missing test coverage.

PR-URL: #29970
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 16, 2019

Landed in e22efba.

Thanks for the contribution! 🎉

@Trott Trott closed this Oct 16, 2019
MylesBorins added a commit that referenced this pull request Oct 23, 2019
Provides some missing test coverage.

PR-URL: #29970
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
targos added a commit that referenced this pull request Nov 8, 2019
Provides some missing test coverage.

PR-URL: #29970
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Nov 10, 2019
Provides some missing test coverage.

PR-URL: #29970
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.