test: increase test coverage for fs.promises read#22800
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Trott commented Sep 13, 2018
Trott commented Sep 13, 2018
Welcome @ratracegrad, and thanks for the pull request! /pinging @nodejs/fs @nodejs/testing for reviews |
addaleax left a comment
addaleax left a comment There was a problem hiding this comment.
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 commented Sep 14, 2018
LGTM. I'm able to confirm that this covers https://coverage.nodejs.org/coverage-1cee08536794b6d7/root/internal/fs/promises.js.html#L123. |
Trott left a comment
Trott left a comment There was a problem hiding this comment.
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 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 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 |
bcoe left a comment
bcoe left a comment There was a problem hiding this comment.
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 commented Sep 28, 2018
Trott commented Sep 28, 2018
mhdawson commented Sep 28, 2018
CI was good, landing |
mhdawson commented Sep 28, 2018
Landed as 27f3d9a. @ratracegrad thanks :) |
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]>
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]>
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]>
8 participants
Increased test coverage for fs.promises by hitting missing branch on read.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes