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

Add tests into internal/crypto/random.js to improve coverage #17555

Closed
wants to merge 4 commits into from
Closed

Add tests into internal/crypto/random.js to improve coverage #17555

wants to merge 4 commits into from

Conversation

Leko
Copy link
Contributor

@Leko Leko commented Dec 8, 2017

I added these test case:

  • Call randomBytes with cb is not a function
  • Call randomFill with cb is not a function

Current coverage is here: https://coverage.nodejs.org/coverage-06e1b0386196f8f8/root/internal/crypto/random.js.html

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 8, 2017
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

This seems fine but you could make it even better by using something similar to line 510. That is, having a forEach that iterates over several different types and makes sure that none of them are accepted for the callback param.

The current version makes sure that null isn't accepted but if someone accidentally changed the function to have a falsey check instead of typeof then it wouldn't catch that.

@Leko
Copy link
Contributor Author

Leko commented Dec 9, 2017

@apapirovski Thank you for review.

using something similar to line 510

I think it would be nice.
I updated test case. Please check it again.

@@ -487,7 +487,17 @@ common.expectsError(
}
);

common.expectsError(
() => crypto.randomBytes(1, null),
Copy link
Member

Choose a reason for hiding this comment

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

Could this also be put inside the forEach loop? Or have a new forEach loop around it? Then we're good to go. :)

Copy link
Contributor Author

@Leko Leko Dec 10, 2017

Choose a reason for hiding this comment

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

I moved it to inside the new forEach loop to omit undefined in test case.

function randomBytes(size, cb) {
  assertSize(size);
  Iif (cb !== undefined && typeof cb !== 'function') // <- undefined is omitted
    throw new errors.TypeError('ERR_INVALID_CALLBACK');

@apapirovski
Copy link
Member

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

@apapirovski
Copy link
Member

apapirovski commented Dec 12, 2017

Landed in 7a055f1

apapirovski pushed a commit that referenced this pull request Dec 12, 2017
- Call randomBytes with a non-function callback
- Call randomFill with a non-function callback

PR-URL: #17555
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Leko Leko deleted the add_test_to_improve_crypto-random_coverage branch December 13, 2017 04:34
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
- Call randomBytes with a non-function callback
- Call randomFill with a non-function callback

PR-URL: #17555
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
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.

6 participants