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: improve crypto coverage #11280

Closed
wants to merge 1 commit into from
Closed

test: improve crypto coverage #11280

wants to merge 1 commit into from

Conversation

akito0107
Copy link
Contributor

@akito0107 akito0107 commented Feb 10, 2017 •
edited
Loading

This PR improves test coverage on crypto includes as below:

  • call Certificate function directly
  • check exception when sign option is undefined
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)

N/A

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 10, 2017
@hiroppy hiroppy added the crypto Issues and PRs related to the crypto subsystem. label Feb 10, 2017
Trott
Trott reviewed Feb 10, 2017
View reviewed changes
test/parallel/test-crypto-sign-verify.js Outdated
{
assert.throws(() => {
crypto.createSign('RSA-SHA1').update('Test123').sign(null, 'base64');
}, /No key provided to sign/);
Copy link
Member

Choose a reason for hiding this comment

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

Can you change the regular expression to match the full error message? /^Error: No key provided to sign$/

@Trott
Copy link
Member

Trott commented Feb 10, 2017

@nodejs/testing

Trott
Trott requested changes Feb 10, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Left a request for a change to the regular expression but other than that, looks good to me. Thanks for the contribution!

@akito0107 akito0107 force-pushed the improve-test-crypto-coverage branch from 0d59852 to fc0595e Compare February 10, 2017 05:53
@akito0107
Copy link
Contributor Author

@Trott Thank you for your review! I fixed!

Trott
Trott approved these changes Feb 10, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@santigimeno
Copy link
Member

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

@jasnell
Copy link
Member

jasnell commented Feb 13, 2017

This will need to be updated now that the pbkdf2 deprecation has been replaced with a throw

@akito0107
test: improve crypto coverage
5e7dbe1 
- call Certificate function directly
- check exception when sign option is undefined
@akito0107 akito0107 force-pushed the improve-test-crypto-coverage branch from fc0595e to 5e7dbe1 Compare February 15, 2017 07:17
@akito0107
Copy link
Contributor Author

@jasnell
Thanks, I fixed!

@yosuke-furukawa
Copy link
Member

So, this PR would be better to add some labels like dont-land-on-vXX ?

@jasnell
Copy link
Member

jasnell commented Feb 15, 2017

This PR does not depend on #11305 so it shouldn't need dont-land-on labels, and that PR is already marked semver-major so dont-land is implicit.

jasnell pushed a commit that referenced this pull request Feb 15, 2017
@akito0107 @jasnell
test: improve crypto coverage
036eef1 
* call Certificate function directly
* check exception when sign option is undefined

PR-URL: #11280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Feb 15, 2017

Landed in 036eef1

@jasnell jasnell closed this Feb 15, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 16, 2017
@akito0107 @italoacasas
test: improve crypto coverage
f27d5c5 
* call Certificate function directly
* check exception when sign option is undefined

PR-URL: nodejs#11280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
@akito0107 @italoacasas
test: improve crypto coverage
348f2ef 
* call Certificate function directly
* check exception when sign option is undefined

PR-URL: #11280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@akito0107 @jasnell
test: improve crypto coverage
d6093e7 
* call Certificate function directly
* check exception when sign option is undefined

PR-URL: #11280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@akito0107 @jasnell
test: improve crypto coverage
dbcae5a 
* call Certificate function directly
* check exception when sign option is undefined

PR-URL: #11280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@akito0107 @MylesBorins
test: improve crypto coverage
92f6919 
* call Certificate function directly
* check exception when sign option is undefined

PR-URL: #11280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@akito0107 @MylesBorins
test: improve crypto coverage
d729e52 
* call Certificate function directly
* check exception when sign option is undefined

PR-URL: #11280
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@santigimeno santigimeno santigimeno approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

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

Successfully merging this pull request may close these issues.
8 participants
@akito0107 @Trott @santigimeno @jasnell @yosuke-furukawa @cjihrig @hiroppy @nodejs-github-bot