-
Notifications
You must be signed in to change notification settings - Fork 29k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test:parameters of fs read and readsync #17910
Conversation
const fs = require('fs'); | ||
const filepath = fixtures.path('x.txt'); | ||
const fd = fs.openSync(filepath, 'r'); | ||
const expected = Buffer.from('xyz\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this buffer needed? It seems that only its length is used below.
I would replace it with const length = 4;
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed it
|
||
[true, null, undefined, () => {}, {}].forEach((value) => { | ||
common.expectsError(() => { | ||
fs.read(value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fs.readSync
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, sorry, I have fixed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general but adding a check for the message in all cases would be nice.
length, | ||
0, | ||
common.mustNotCall()); | ||
}, { code: 'ERR_INVALID_ARG_TYPE', type: TypeError }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to test for the error message as well to make sure the right error is triggered.
added test-fs-read-sync-constructor-errors and test-fs-read-constructor-errors tests added to extend the code coverage testing of the project
added test-fs-read-sync-constructor-errors and test-fs-read-constructor-errors tests added to extend the code coverage testing of the project
added test-fs-read-sync-constructor-errors and test-fs-read-constructor-errors tests added to extend the code coverage testing of the project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to move these test cases to test/parallel/test-fs-read-type.js
? It looks like there are already some type checking tests for fs.read
and fs.readSync
there.
Ping @bacriom , do you have the time to address #17910 (review) ? |
sorry for the answer too late, ok, I'll move my tests cases to |
80c6c12
to
f4e9a87
Compare
Test failures are not related |
Added tests to extend the code coverage. PR-URL: nodejs#17910 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>
This does not land cleanly on v9.x, would a collaborator be willing to backport? |
Added tests to extend the code coverage. PR-URL: nodejs#17910 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>
added test-fs-read-sync-constructor-errors and test-fs-read-constructor-errors
tests added to extend the code coverage testing of the project.
ref: pull closed #17875
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)