test: increase test coverage for fs.promises read#22800
test: increase test coverage for fs.promises read#22800ratracegrad wants to merge 4 commits intonodejs:masterfrom codeprep:fs-promises
Conversation
|
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).
|
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?
|
@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. |
|
@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.
|
CI was good, landing |
|
Landed as 27f3d9a. @ratracegrad thanks :) |
PR-URL: #22800 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #22800 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #22800 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
8 participants
Increased test coverage for fs.promises by hitting missing branch on read.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes