test: add blocks and comments to test cases in test-fs-promises #23627
Conversation
|
@bcoe Please review. |
| async () => { | ||
| await chown(dest, 1, -1); | ||
| }, | ||
| { |
There was a problem hiding this comment.
The error object on line 158 and 169 is same and can be stored in a variable instead:
{
code: 'ERR_OUT_OF_RANGE',
name: 'RangeError [ERR_OUT_OF_RANGE]',
message: 'The value of "gid" is out of range. ' +
'It must be >= 0 && < 4294967296. Received -1'
}
There was a problem hiding this comment.
this is looking pretty good to me, a couple notes:
- I think the one thing being tested is
truncatenotreadFileat a glance. - if possible, let's see about creating a new file handle for each test; to eliminate shared state between tests.
I'll give a final read-through when we're getting closer to done, to make sure nothing else jumps out at me like truncate; thanks for taking this work on, this is an awesome first pull request.
test/parallel/test-fs-promises.js
Outdated
| if (!common.isWindows) { | ||
| await chown(dest, process.getuid(), process.getgid()); | ||
| await handle.chown(process.getuid(), process.getgid()); | ||
| // async read from file |
There was a problem hiding this comment.
I would say that this is asserting that "reading from buffer is identical to bytes written" ... whereas.
test/parallel/test-fs-promises.js
Outdated
| code: 'ENOSYS', | ||
| type: Error | ||
| })(err); | ||
| // async read from file handle |
There was a problem hiding this comment.
this is actually asserting that truncate behaves as expected, removing the end of the file.
|
@ratracegrad would you be able to provide some code review for this, since you're familiar with this file? |
|
@iansu if you run the following: and rewrite your commit message to: ☝️ this should dance around the linting issues, and I can kick off tests again. |
iansu
force-pushed
the
test-fs-promises-cleanup
branch
from
96afb47
to
bb6c83b
Compare
|
@bcoe Done. |
|
It seems like that's stalled waiting on a |
|
@nodejs/testing these failures look unrelated to @iansu's improvements, I intend to land this later today or tomorrow. |
Please do not land without a green or yellow CI. Doing so encourages ignoring rather than addressing CI failures. It also violates our documented process. Use the "Resume Build" feature (in the left nav) on a node-test-pull-request run to re-run only the things that aren't green. Resume Build: https://ci.nodejs.org/job/node-test-pull-request/18016/ |
|
Looks like it worked this time. |
PR-URL: nodejs#23627 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
refack
force-pushed
the
test-fs-promises-cleanup
branch
from
bb6c83b
to
3b7f9bc
Compare
|
Landed in 3b7f9bc 🎉 |
PR-URL: #23627 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #23627 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #23627 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #23627 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes