test: improve the code in test-crypto-rsa-dsa by edsadr · Pull Request #10681 · nodejs/node

edsadr · 2017-01-08T02:43:19Z

use const and let instead of var
use assert.strictEqual instead of assert.equal
use arrow functions
swap assertions arguments to match the standard

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

Trott · 2017-01-08T07:02:29Z

There are five instances of assert.throws() that get just one argument (the function containing code to run). While you're in there improving the test, it would be great if you could add a second argument containing a RegExp that matches the resulting error message.

cjihrig · 2017-01-08T19:42:05Z

Could you name this expectedSignature?

edsadr · 2017-01-09T02:08:18Z

@Trott , @cjihrig , did as suggested

lpinca · 2017-01-09T07:50:20Z

Nit: isn't it better to compare the two buffers directly instead of their string representation?
assert.deepStrictEqual(decryptedBuffer,input);
or
assert(decryptedBuffer.equals(input));

lpinca · 2017-01-09T07:52:06Z

Nit: I think should be shifted to the left by 2 spaces.

* use const and let instead of var * use assert.strictEqual or assert.strictDeepEqual instead of assert.equal * use arrow functions * swap assertions arguments to match the standard * validate the error for assert.throws

edsadr · 2017-01-09T16:34:27Z

@lpinca , did as suggested

lpinca · 2017-01-09T17:17:21Z

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

lpinca · 2017-01-10T16:56:36Z

Landed in 85a9034.

* use const and let instead of var * use assert.strictEqual or assert.strictDeepEqual instead of assert.equal * use arrow functions * swap assertions arguments to match the standard * validate the error for assert.throws PR-URL: #10681 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

* use const and let instead of var * use assert.strictEqual or assert.strictDeepEqual instead of assert.equal * use arrow functions * swap assertions arguments to match the standard * validate the error for assert.throws PR-URL: nodejs#10681 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

* use const and let instead of var * use assert.strictEqual or assert.strictDeepEqual instead of assert.equal * use arrow functions * swap assertions arguments to match the standard * validate the error for assert.throws PR-URL: #10681 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

* use const and let instead of var * use assert.strictEqual or assert.strictDeepEqual instead of assert.equal * use arrow functions * swap assertions arguments to match the standard * validate the error for assert.throws PR-URL: nodejs#10681 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

jasnell · 2017-03-08T20:51:38Z

This will need backport PRs in order to land on v6 or v4

* use const and let instead of var * use assert.strictEqual or assert.strictDeepEqual instead of assert.equal * use arrow functions * swap assertions arguments to match the standard * validate the error for assert.throws PR-URL: #10681 Backport-PR-URL: #13785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

* use const and let instead of var * use assert.strictEqual or assert.strictDeepEqual instead of assert.equal * use arrow functions * swap assertions arguments to match the standard * validate the error for assert.throws PR-URL: nodejs/node#10681 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Jan 8, 2017

mscdex added crypto Issues and PRs related to the crypto subsystem. and removed dont-land-on-v7.x labels Jan 8, 2017

jasnell approved these changes Jan 8, 2017
View reviewed changes

cjihrig reviewed Jan 8, 2017
View reviewed changes

Comment threadtest/parallel/test-crypto-rsa-dsa.js Outdated
cjihrigJan 8, 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.
Could you name this expectedSignature?

cjihrig approved these changes Jan 8, 2017
View reviewed changes

edsadr force-pushed the crypto-rsa branch from 3551abc to d5b79c0Compare January 9, 2017 01:30

nodejs-github-bot added the dont-land-on-v7.x label Jan 9, 2017

mscdex removed the dont-land-on-v7.x label Jan 9, 2017

lpinca reviewed Jan 9, 2017
View reviewed changes

Comment threadtest/parallel/test-crypto-rsa-dsa.js Outdated
lpincaJan 9, 2017
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think should be shifted to the left by 2 spaces.

test: improve the code in test-crypto-rsa-dsa
faa4e29
* use const and let instead of var * use assert.strictEqual or assert.strictDeepEqual instead of assert.equal * use arrow functions * swap assertions arguments to match the standard * validate the error for assert.throws

edsadr force-pushed the crypto-rsa branch from d5b79c0 to faa4e29Compare January 9, 2017 16:33

nodejs-github-bot added the dont-land-on-v7.x label Jan 9, 2017

lpinca removed the dont-land-on-v7.x label Jan 9, 2017

lpinca approved these changes Jan 9, 2017
View reviewed changes

lpinca closed this Jan 10, 2017

italoacasas mentioned this pull request Jan 25, 2017
Proposal to update v7.x-staging #10989
Closed

italoacasas mentioned this pull request Jan 29, 2017
v7.5.0 proposal #11062
Merged

jasnell added dont-land-on-v4.x labels Mar 8, 2017

targos mentioned this pull request Jun 19, 2017
(v6.x backport) tools: enforce two arguments in assert.throws #13785
Closed
2 tasks

gibfahn added backport-requested-v6.x and removed dont-land-on-v6.x labels Jun 19, 2017

MylesBorins mentioned this pull request Jul 18, 2017
v6.11.2 proposal #14356
Merged

Navigation Menu

Uh oh!

Uh oh!

test: improve the code in test-crypto-rsa-dsa - #10681

test: improve the code in test-crypto-rsa-dsa#10681
edsadr wants to merge 1 commit into
nodejs:masterfrom
edsadr:crypto-rsa

edsadr commented Jan 8, 2017

Uh oh!

Trott commented Jan 8, 2017

Uh oh!

cjihrigJan 8, 2017

Uh oh!

edsadr commented Jan 9, 2017

Uh oh!

lpincaJan 9, 2017

Uh oh!

lpincaJan 9, 2017

Uh oh!

edsadr commented Jan 9, 2017

Uh oh!

lpinca commented Jan 9, 2017

Uh oh!

lpinca commented Jan 10, 2017

Uh oh!

jasnell commented Mar 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Uh oh!

Conversation

edsadr commented Jan 8, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Trott commented Jan 8, 2017

Uh oh!

cjihrigJan 8, 2017

Choose a reason for hiding this comment

Uh oh!

edsadr commented Jan 9, 2017

Uh oh!

lpincaJan 9, 2017

Choose a reason for hiding this comment

Uh oh!

lpincaJan 9, 2017

Choose a reason for hiding this comment

Uh oh!

edsadr commented Jan 9, 2017

Uh oh!

lpinca commented Jan 9, 2017

Uh oh!

lpinca commented Jan 10, 2017

Uh oh!

jasnell commented Mar 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants