test: add fs/promises filehandle sync() and datasync() tests.#20530
test: add fs/promises filehandle sync() and datasync() tests.#20530shisama wants to merge 4 commits intonodejs:masterfrom
Conversation
There was a problem hiding this comment.
handle.write(buf)?
Same results and coverage, also see #20559.
There was a problem hiding this comment.
read(handle → handle.read(? See the comment above.
ChALkeR
added
experimental
shisama
force-pushed
the
test-fs-promises-file-handle-sync
branch
from
1ff5b56 to
e8f8408
Compare
|
/cc @Trott |
shisama
force-pushed
the
test-fs-promises-file-handle-sync
branch
from
5784c2d to
2b54d4a
Compare
|
This is rebased on 0300f7c. |
|
@nodejs/fs please give this a look and a review, if possible. It's been over a month. |
joyeecheung
left a comment
joyeecheung
left a comment
There was a problem hiding this comment.
This does not seem to actually test that fsync and fdatasync calls are effective, but I am not quite sure how to write tests for them with our APIs either, so LGTM as long as the code paths are covered.
There was a problem hiding this comment.
common.mustCall() here seems unnecessary. If anything, common.crashOnUnhandledRejection() above gives a better hint about why it fails.
There was a problem hiding this comment.
@joyeecheung Thanks. Removed common.mustCall().
To increase test coverage for fs/promises, added tests for filehandle.sync and filehandle.datasync.
Replacing fs/promises methods read and write with fs/promises filehandle methods.
Fix module loading because fs/promises was moved to fs.promises
shisama
force-pushed
the
test-fs-promises-file-handle-sync
branch
from
2b54d4a to
d462d2c
Compare
apapirovski
added
the
author ready
|
CI failure is not related to this. It happens in |
|
I think CI failure |
To increase test coverage for fs.promises, added tests for filehandle.sync and filehandle.datasync. PR-URL: nodejs#20530 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
Landed in 71ca076. Thanks for the contribution @shisama. |
targos
removed
the
author ready
To increase test coverage for fs.promises, added tests for filehandle.sync and filehandle.datasync. PR-URL: #20530 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
10 participants
To increase test coverage for fs/promises,
added tests for filehandle.sync and filehandle.datasync.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes