FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: improve code in test-crypto-verify by edsadr · Pull Request #10845 · nodejs/node · GitHub

/ node Public

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

Closed
edsadr wants to merge 1 commit into
nodejs:masterfrom
edsadr:crypto-verify
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

edsadr 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-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Jan 17, 2017
mscdex added crypto Issues and PRs related to the crypto subsystem. and removed dont-land-on-v7.x labels Jan 17, 2017
cjihrig requested changes Jan 17, 2017
View reviewed changes
Comment thread test/parallel/test-crypto-verify-failure.js Outdated

cjihrig Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

cjihrig Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please move this back to the previous line too.

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

sam-github Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

one .on( per line, please.

edsadr commented Jan 20, 2017

Copy link
Copy Markdown
Contributor Author

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

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

cjihrig Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

cjihrig approved these changes Jan 20, 2017
View reviewed changes

cjihrig 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with one comment.

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 commented Jan 20, 2017

Copy link
Copy Markdown
Contributor Author

@cjihrig did the last Nit too

targos commented Jan 22, 2017

Copy link
Copy Markdown
Member

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

sam-github commented Jan 23, 2017

Copy link
Copy Markdown
Contributor

Landed in 8ab561b

sam-github closed this Jan 23, 2017
sam-github pushed a commit that referenced this pull request Jan 23, 2017
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>
italoacasas mentioned this pull request Jan 25, 2017
Closed
targos pushed a commit that referenced this pull request Jan 28, 2017
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>
italoacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
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
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
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
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
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>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
+2 more reviewers
sam-github sam-github approved these changes
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

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.

7 participants

Back | FazBrowse Home | New Git URL