test: improve crypto.setEngine coverage to check for errors #11143

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@seppevs
Contributor

seppevs commented Feb 3, 2017

test: improve crypto.setEngine test coverage to check for errors

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

@hiroppy hiroppy added the crypto label Feb 3, 2017

@addaleax

LGTM with a green CI & linter

test/parallel/test-crypto-engine.js
+}, /"id" argument should be a string/);
+
+assert.throws(function() {
+ crypto.setEngine('/path/to/engine', 'notANumber')

This comment has been minimized.

@addaleax

addaleax Feb 3, 2017

Member

Does the linter pass with these (make lint)? We do use semicolons to end statements 😄

@addaleax

addaleax Feb 3, 2017

Member

Does the linter pass with these (make lint)? We do use semicolons to end statements 😄

test/parallel/test-crypto-engine.js
+
+assert.throws(function() {
+ crypto.setEngine(true)
+}, /"id" argument should be a string/);

This comment has been minimized.

@hiroppy

hiroppy Feb 3, 2017

Member

You can write /^TypeError: "id" argument should be a string$/ :)

@hiroppy

hiroppy Feb 3, 2017

Member

You can write /^TypeError: "id" argument should be a string$/ :)

@seppevs

This comment has been minimized.

Show comment
Hide comment
@seppevs

seppevs Feb 3, 2017

Contributor

I did the changes:

  • All statements end with a semicolon
  • I check for TypeError in both tests now

I amended the changes to the previous commit.

Contributor

seppevs commented Feb 3, 2017

I did the changes:

  • All statements end with a semicolon
  • I check for TypeError in both tests now

I amended the changes to the previous commit.

@hiroppy

hiroppy approved these changes Feb 3, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@shigeki

This comment has been minimized.

Show comment
Hide comment
@shigeki

shigeki Feb 3, 2017

Contributor

This is duplicated to #10786 and related #10865. I'm now waiting for resolve nodejs/build#596.

Contributor

shigeki commented Feb 3, 2017

This is duplicated to #10786 and related #10865. I'm now waiting for resolve nodejs/build#596.

@jasnell

jasnell approved these changes Feb 7, 2017

jasnell added a commit that referenced this pull request Feb 7, 2017

test: improve crypto.setEngine coverage to check for errors
PR-URL: #11143
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 7, 2017

Member

Landed in ddbfdba

Member

jasnell commented Feb 7, 2017

Landed in ddbfdba

@jasnell jasnell closed this Feb 7, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 9, 2017

test: improve crypto.setEngine coverage to check for errors
PR-URL: nodejs#11143
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: improve crypto.setEngine coverage to check for errors
PR-URL: nodejs#11143
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

test: improve crypto.setEngine coverage to check for errors
PR-URL: nodejs#11143
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Mar 7, 2017

test: improve crypto.setEngine coverage to check for errors
PR-URL: #11143
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

Would require a backport PR to land on v4

Member

jasnell commented Mar 7, 2017

Would require a backport PR to land on v4

MylesBorins added a commit that referenced this pull request Mar 9, 2017

test: improve crypto.setEngine coverage to check for errors
PR-URL: #11143
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

@shigeki shigeki referenced this pull request Mar 25, 2017

Closed

test: add crypto's setEngine test #10786

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment