test: increase test coverage for fs.promises read #22800
Conversation
|
Welcome @ratracegrad, and thanks for the pull request! /pinging @nodejs/fs @nodejs/testing for reviews |
|
LGTM. I'm able to confirm that this covers https://coverage.nodejs.org/coverage-1cee08536794b6d7/root/internal/fs/promises.js.html#L123. |
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 |
|
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>
Increased test coverage for fs.promises by hitting missing branch on read.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes