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.

Sign up for GitHub

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

Jump to bottom

test: add coverage for uncovered if statement in fs.read function(callback and promise api) #35918

Closed
wants to merge 0 commits into from
Closed

test: add coverage for uncovered if statement in fs.read function(callback and promise api) #35918

wants to merge 0 commits into from
+0 −0

Conversation

mayankagarwals
Copy link
Contributor

@mayankagarwals mayankagarwals commented Nov 2, 2020 •
edited
Loading

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 •
edited
Loading

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 mayankagarwals mentioned this pull request Nov 4, 2020
Closed
@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.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott closed this Nov 17, 2020
@Trott
Copy link
Member

Trott commented Nov 17, 2020

Landed in 0027aca

Trott pushed a commit that referenced this pull request Nov 17, 2020
@mayankagarwals @Trott
test: add test for fs.read when offset key is null
0027aca 
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
@mayankagarwals @codebytere
test: add test for fs.read when offset key is null
c19df17 
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
Merged
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@mayankagarwals @BethGriggs
test: add test for fs.read when offset key is null
a3ba358 
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
@mayankagarwals @BethGriggs
test: add test for fs.read when offset key is null
797e75f 
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
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@mayankagarwals @BethGriggs
test: add test for fs.read when offset key is null
b6aa42c 
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>
@LiviaMedeiros LiviaMedeiros mentioned this pull request Apr 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mayankagarwals @Trott @nodejs-github-bot @joyeecheung