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: add coverage for uncovered if statement in fs.read function(callback and promise api) #35918

Closed

Conversation

mayankagarwals
Copy link
Contributor

@mayankagarwals mayankagarwals commented Nov 2, 2020

Expanded coverage of tests as my first contribution to nodejs:). It would be really helpful if other contributors can point out any errors I have made and/or good practices I am missing!.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 2, 2020
@Trott
Copy link
Member

Trott commented Nov 2, 2020

Welcome @iammack and thanks for the pull request.

Is this the line you're adding coverage for?

https://coverage.nodejs.org/coverage-09af8c822c8e931b/lib/fs.js.html#L514

If so, we probably also want to test that it works when offset is undefined.

@mayankagarwals
Copy link
Contributor Author

mayankagarwals commented Nov 2, 2020

Glad to be contributing:)

Yes, that line and this line: https://coverage.nodejs.org/coverage-642f2064c06793b7/lib/internal/fs/promises.js.html#L326.

Actually, the test when offset is undefined has been implemented in https://github.com/nodejs/node/blob/master/test/parallel/test-fs-read-optional-params.js but this test just compares the length of the returned buffer and not its contents. Is that a good enough test? If not, wouldn't it be better to change the optional-params file itself to include a check for contents so that the change would cover other parameters of fs.read too making tests for all the conditions stronger?

@mayankagarwals
Copy link
Contributor Author

Not sure what's the equivalent of "bringing this to the top of your inbox" in GitHub :p. So gentle ping @Trott

@Trott
Copy link
Member

Trott commented Nov 6, 2020

I'm surprised that setting offset to undefined doesn't cover the same line, but I also didn't look that closely. Further improvements are welcome, please don't let me stop you, but this is good enough to land. Thanks.

@mayankagarwals mayankagarwals force-pushed the test-fs-read-offset-null branch 2 times, most recently from 852a036 to d7176d4 Compare November 9, 2020 11:30
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Nov 17, 2020

Landed in 0027aca

Trott pushed a commit that referenced this pull request Nov 17, 2020
added test for uncovered if statement in lib/fs.js

PR-URL: #35918
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
codebytere pushed a commit that referenced this pull request Nov 22, 2020
added test for uncovered if statement in lib/fs.js

PR-URL: #35918
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@codebytere codebytere mentioned this pull request Nov 22, 2020
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
added test for uncovered if statement in lib/fs.js

PR-URL: #35918
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
added test for uncovered if statement in lib/fs.js

PR-URL: #35918
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
added test for uncovered if statement in lib/fs.js

PR-URL: #35918
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants