Skip to content
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: error when empty buffer is passed to filehandle.read() #23250

Closed

Conversation

shisama
Copy link
Contributor

@shisama shisama commented Oct 3, 2018

Added tests to occur error when empty buffer is passed to filehandle.read()
to increase coverage.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 3, 2018
@@ -26,3 +27,14 @@ assert.throws(
'Received Uint8Array []'
}
);

fsPromises.open(filepath, 'r').then((filehandle) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: if we could write this with await it would look better.

@@ -26,3 +27,14 @@ assert.throws(
'Received Uint8Array []'
}
);

fsPromises.open(filepath, 'r').then((filehandle) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, now that you are doing it, please cover the fs promises read with a file descriptor.

{
code: 'ERR_INVALID_ARG_VALUE',
message: 'The argument \'buffer\' is empty and cannot be written. ' +
'Received Uint8Array []'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think this needs to be aligned with 'The argument...

@shisama shisama force-pushed the test-fsPromises-readfile-empty-buffer branch from 61d6bef to a93f6e0 Compare October 6, 2018 15:17
@danbev
Copy link
Contributor

danbev commented Oct 10, 2018

@shisama Could you update the commit message and correct the length as reported by travis?

CI: https://ci.nodejs.org/job/node-test-pull-request/17719/

@shisama shisama force-pushed the test-fsPromises-readfile-empty-buffer branch from a93f6e0 to 3b01a5e Compare October 10, 2018 07:34
@shisama
Copy link
Contributor Author

shisama commented Oct 10, 2018

@danbev Thank you for your comment. I updated the commit message.

@danbev
Copy link
Contributor

danbev commented Oct 10, 2018

@shisama Sorry about this, but could I also ask you to rebase this to fix this travis CI failure? (this is related to #23397).

Masashi Hirano added 3 commits October 10, 2018 18:10
@shisama shisama force-pushed the test-fsPromises-readfile-empty-buffer branch from 3b01a5e to 41afe00 Compare October 10, 2018 09:10
@danbev
Copy link
Contributor

danbev commented Oct 10, 2018

Re-run of failing node-test-commit-linux-containered ✔️

@shisama
Copy link
Contributor Author

shisama commented Oct 10, 2018

@danbev Thank you to tell me that this should be rebased and to run CI.

@danbev
Copy link
Contributor

danbev commented Oct 10, 2018

Landed in a8530bc.

@danbev danbev closed this Oct 10, 2018
danbev pushed a commit that referenced this pull request Oct 10, 2018
Added tests to occur error when empty buffer is passed to
filehandle.read() to increase coverage.

PR-URL: #23250
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos added this to Don't land (ever) in v10.x Oct 16, 2018
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Added tests to occur error when empty buffer is passed to
filehandle.read() to increase coverage.

PR-URL: #23250
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
No open projects
v10.x
  
Don't land (ever)
Development

Successfully merging this pull request may close these issues.

None yet

9 participants