Skip to content

test: improve code in test-crypto-verify - #10845

Closed
edsadr wants to merge 1 commit into
nodejs:masterfrom
edsadr:crypto-verify
Closed

test: improve code in test-crypto-verify#10845
edsadr wants to merge 1 commit into
nodejs:masterfrom
edsadr:crypto-verify

Conversation

@edsadr

@edsadredsadr commented Jan 17, 2017

Copy link
Copy Markdown
Contributor
  • use common.mustCall to validate functions executions
  • use common.fail to check test fail
  • remove console.log
  • use arrow functions
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-botnodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Jan 17, 2017
@mscdexmscdex added crypto Issues and PRs related to the crypto subsystem. and removed dont-land-on-v7.x labels Jan 17, 2017
cjihrig
cjihrig requested changes Jan 17, 2017
View reviewed changes
Comment threadtest/parallel/test-crypto-verify-failure.js Outdated

@cjihrigcjihrigJan 17, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you move this curly brace back to the previous line.

Comment threadtest/parallel/test-crypto-verify-failure.js Outdated

@cjihrigcjihrigJan 17, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please move this back to the previous line too.

sam-github
sam-github suggested changes Jan 17, 2017
View reviewed changes
Comment threadtest/parallel/test-crypto-verify-failure.js Outdated

@sam-githubsam-githubJan 17, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one .on( per line, please.

@edsadr

edsadr commented Jan 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@cjihrig , @sam-github, did both changes please check

cjihrig
cjihrig reviewed Jan 20, 2017
View reviewed changes
Comment threadtest/parallel/test-crypto-verify-failure.js Outdated

@cjihrigcjihrigJan 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess technically, this should be wrapped in a common.mustCall() to make sure the inner common.mustCall()s are executed.

cjihrig
cjihrig approved these changes Jan 20, 2017
View reviewed changes

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with one comment.

@edsadr
test: improve code in test-crypto-verify
67b920d
* use common.mustCall to validate functions executions
* use common.fail to check test fail
* remove console.log
* use arrow functions
@edsadr

edsadr commented Jan 20, 2017

Copy link
Copy Markdown
ContributorAuthor

@cjihrig did the last Nit too

@targos

targos commented Jan 22, 2017

Copy link
Copy Markdown
Member

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

@sam-github

sam-github commented Jan 23, 2017

Copy link
Copy Markdown
Contributor

Landed in 8ab561b

sam-github pushed a commit that referenced this pull request Jan 23, 2017
@edsadr@sam-github
test: improve code in test-crypto-verify
8ab561b
* use common.mustCall to validate functions executions
* use common.fail to check test fail
* remove console.log
* use arrow functions
PR-URL: #10845
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasasitaloacasas mentioned this pull request Jan 25, 2017
Closed
targos pushed a commit that referenced this pull request Jan 28, 2017
@edsadr@targos
test: improve code in test-crypto-verify
6ed725c
* use common.mustCall to validate functions executions
* use common.fail to check test fail
* remove console.log
* use arrow functions
PR-URL: #10845
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasasitaloacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@edsadrItalo A. Casas
test: improve code in test-crypto-verify
eb7ee50
* use common.mustCall to validate functions executions
* use common.fail to check test fail
* remove console.log
* use arrow functions
PR-URL: nodejs#10845
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@edsadrItalo A. Casas
test: improve code in test-crypto-verify
3b00ecc
* use common.mustCall to validate functions executions
* use common.fail to check test fail
* remove console.log
* use arrow functions
PR-URL: nodejs#10845
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
@edsadr@jasnell
test: improve code in test-crypto-verify
5e5a38d
* use common.mustCall to validate functions executions
* use common.fail to check test fail
* remove console.log
* use arrow functions
PR-URL: #10845
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
@edsadr@jasnell
test: improve code in test-crypto-verify
24abcaa
* use common.mustCall to validate functions executions
* use common.fail to check test fail
* remove console.log
* use arrow functions
PR-URL: #10845
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@edsadr@MylesBorins
test: improve code in test-crypto-verify
0c60540
* use common.mustCall to validate functions executions
* use common.fail to check test fail
* remove console.log
* use arrow functions
PR-URL: #10845
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@edsadr@MylesBorins
test: improve code in test-crypto-verify
d8af5a7
* use common.mustCall to validate functions executions
* use common.fail to check test fail
* remove console.log
* use arrow functions
PR-URL: #10845
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@targostargostargos approved these changes
@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@sam-githubsam-githubsam-github approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cryptoIssues and PRs related to the crypto subsystem.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@edsadr@targos@sam-github@jasnell@cjihrig@mscdex@nodejs-github-bot