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: increase test coverage for fs.promises read #22800

Closed
wants to merge 4 commits into from
Closed

test: increase test coverage for fs.promises read #22800

wants to merge 4 commits into from

Conversation

ratracegrad
Copy link
Contributor

@ratracegrad ratracegrad commented Sep 11, 2018

Increased test coverage for fs.promises by hitting missing branch on read.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 11, 2018
@bcoe bcoe self-requested a review September 11, 2018 02:04
@Trott
Copy link
Member

Trott commented Sep 13, 2018

@Trott
Copy link
Member

Trott commented Sep 13, 2018

Welcome @ratracegrad, and thanks for the pull request!

/pinging @nodejs/fs @nodejs/testing for reviews

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

As an optional suggestion, you could also check that ret.buffer remains unchanged here (I think in this case that means it only contains 0 bytes).

@Trott
Copy link
Member

Trott commented Sep 14, 2018

Trott
Trott previously requested changes Sep 14, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Er, uh, maybe not. The test fails, probably because using the handle changes the results for the next test case? To avoid side effects, I imagine you'll need to create a new handle for this test, or flush the data you loaded in there or put it after other tests?

@bcoe
Copy link
Contributor

bcoe commented Sep 14, 2018

@Trott @ratracegrad the setup between tests seems pretty fragile, assertions rely on the behavior of the prior assertion (this was already the case before Jennifer wrote the new test).

What if we update this test so all its setup is included in:

{
}

including the temp file creation.

@Trott
Copy link
Member

Trott commented Sep 14, 2018

@bcoe Yes, a change to provide a block scope around each test and to not share/re-use anything other than the built-in modules (and common module stuff, if applicable) would be 👍 for me.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

I'm really happy with this; I like that the state has started to be cleaned up between tests, this is good future work.

@ratracegrad
Copy link
Contributor Author

@Trott @addaleax I have updated my PR so that it does not cause following tests to fail. Ready for you to review again.

@Trott
Copy link
Member

Trott commented Sep 28, 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

@mhdawson
Copy link
Member

CI was good, landing

@mhdawson
Copy link
Member

Landed as 27f3d9a. @ratracegrad thanks :)

@mhdawson mhdawson closed this Sep 28, 2018
mhdawson pushed a commit that referenced this pull request Sep 28, 2018
PR-URL: #22800
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Sep 29, 2018
PR-URL: #22800
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Oct 3, 2018
PR-URL: #22800
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants