Skip to content

Conversation

@Trott
Copy link
Member

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

@TrottTrott 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
MemberAuthor

Copy link
Member

@jasnelljasnell 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
MemberAuthor

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

@Trott
Copy link
MemberAuthor

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

@TrottTrottforce-pushed the test-benchmark-crypto branch from f6cd62c to 34de9aaCompareApril 14, 2017 18:22
@Trott
Copy link
MemberAuthor

Add minimal test for crypto benchmarks. It makes sure that they can run without returning an error code.
@TrottTrottforce-pushed the test-benchmark-crypto branch from 34de9aa to ce19d1aCompareApril 14, 2017 18:48
@Trott
Copy link
MemberAuthor

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
MemberAuthor

Landed in 8a6d44b

@TrottTrott 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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
@evanlucasevanlucas 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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
@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

@Trott
Copy link
MemberAuthor

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
MemberAuthor

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

@TrottTrott deleted the test-benchmark-crypto branch January 13, 2022 22:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Trott@MylesBorins@joyeecheung@jasnell@cjihrig