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.

Sign up for GitHub

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

Jump to bottom

test: add test-benchmark-crypto #12347

Closed
wants to merge 1 commit into from
Closed

test: add test-benchmark-crypto #12347

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

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

jasnell
jasnell approved these changes Apr 14, 2017
View reviewed changes
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

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

@Trott
test: add test-benchmark-crypto
ce19d1a 
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
@Trott
test: add test-benchmark-crypto
8a6d44b 
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
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
@Trott @evanlucas
test: add test-benchmark-crypto
6d458b2 
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 evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@Trott @evanlucas
test: add test-benchmark-crypto
81615f2 
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
@Trott @evanlucas
test: add test-benchmark-crypto
0661fdf 
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

@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 •
edited
Loading

@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

@Trott Trott deleted the test-benchmark-crypto branch January 13, 2022 22:45
@baby636 baby636 mentioned this pull request Jun 22, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Trott @MylesBorins @joyeecheung @jasnell @cjihrig