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.

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

test: add test-benchmark-crypto #12347

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 11, 2017

Add minimal test for crypto benchmarks. It makes sure that they can run
without returning an error code.

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 benchmark crypto

@Trott Trott added benchmark Issues and PRs related to the benchmark subsystem. crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests. labels Apr 11, 2017
@Trott
Copy link
Member Author

Trott commented Apr 14, 2017

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Lgtm. There's a fair amount of duplication across these benchmark tests. It may be worthwhile to explore a utility function in common.

@Trott
Copy link
Member Author

Trott commented Apr 14, 2017

Some timeouts and I probably need to add some hasCrypto check to the test too. Revisions coming soon...

@Trott
Copy link
Member Author

Trott commented Apr 14, 2017

It may be worthwhile to explore a utility function in common.

Perhaps even better, maybe let it be a first PoC for a module for a specific type of test (like common-benchmark or something). I'm not a fan of expanding the common monolith even more...

@Trott
Copy link
Member Author

Trott commented Apr 14, 2017

Add minimal test for crypto benchmarks. It makes sure that they can run
without returning an error code.
@Trott
Copy link
Member Author

Trott commented Apr 14, 2017

At least one benchmark using FIPS-incompatible crypto.createCipher() so skip this test on FIPS.

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

Trott added a commit to Trott/io.js that referenced this pull request Apr 14, 2017
Add minimal test for crypto benchmarks. It makes sure that they can run
without returning an error code.

PR-URL: nodejs#12347
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Apr 14, 2017

Landed in 8a6d44b

@Trott Trott closed this Apr 14, 2017
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
Add minimal test for crypto benchmarks. It makes sure that they can run
without returning an error code.

PR-URL: #12347
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
Add minimal test for crypto benchmarks. It makes sure that they can run
without returning an error code.

PR-URL: #12347
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
Add minimal test for crypto benchmarks. It makes sure that they can run
without returning an error code.

PR-URL: #12347
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

@MylesBorins MylesBorins reopened this May 15, 2017
@Trott
Copy link
Member Author

Trott commented May 16, 2017

This does not land cleanly in LTS.

@MylesBorins Are you sure about that? It adds a file that didn't previously exist so I'm not sure how that could be that case. git cherry-pick 8a6d44b lands cleanly for me on v6.x-staging.

@MylesBorins
Copy link
Contributor

more specfiically the test fails... sorry for the rush job before

=== release test-benchmark-crypto ===
Path: parallel/test-benchmark-crypto
module.js:471
    throw err;
    ^

Error: Cannot find module '/Users/mborins/code/node/v6.x/benchmark/run.js'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:389:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:504:3

assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: 1 === 0
    at ChildProcess.child.on (/Users/mborins/code/node/v6.x/test/parallel/test-benchmark-crypto.js:34:10)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:215:12)
Command: out/Release/node /Users/mborins/code/node/v6.x/test/parallel/test-benchmark-crypto.js
[00:00|% 100|+   0|-   1]: Done

@joyeecheung
Copy link
Member

joyeecheung commented May 16, 2017

@MylesBorins The benchmarking tools were rewritten during 7.x period so I think all the test-benchmark-*.js PRs should not be backported to 6.x

@Trott
Copy link
Member Author

Trott commented May 16, 2017

more specfiically the test fails... sorry for the rush job before

@MylesBorins Guess I could have taken a moment to actually run the tests. Got it. Sorry about that! :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants