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: make test-crypto-timing-safe-equal-benchmarks more reliable #9241

Merged
merged 2 commits into from
Oct 24, 2016
Merged

test: make test-crypto-timing-safe-equal-benchmarks more reliable #9241

merged 2 commits into from
Oct 24, 2016

Conversation

bnoordhuis
Copy link
Member

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 22, 2016
@bnoordhuis bnoordhuis mentioned this pull request Oct 22, 2016
Closed
@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Oct 22, 2016
@not-an-aardvark
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-commit/5731/

LGTM if this makes the test less flaky

@not-an-aardvark
Copy link
Contributor

Oh wait, I suppose this test won't actually get run in CI now that it's part of pummel. Anyway, LGTM.

@jasnell
Copy link
Member

jasnell commented Oct 22, 2016

Unfortunate that this won't be run regularly. We'll just need to be vigilant about running the test manually whenever there's a change that could impact this.

@bnoordhuis
test: move flaky test to test/pummel
1b8fca1 
Move sequential/test-crypto-timing-safe-equal-benchmarks to test/pummel
because it fails for me locally quite frequently and because it takes
about five or six seconds to complete, which is too long for a test in
test/sequential.

Fixes: nodejs#8744
PR-URL: nodejs#9241
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
@bnoordhuis
test: make flaky pummel test more reliable
d1f7c96 
Increase the number of iterations from 1e4 to 1e5.  Makes the test pass
for me locally when previously it would fail 9 out of 10 times because
the running time was not enough to smooth away the outliers.

Fixes: nodejs#8744
PR-URL: nodejs#9241
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
@bnoordhuis bnoordhuis closed this Oct 24, 2016
@bnoordhuis bnoordhuis deleted the fix8744 branch October 24, 2016 20:45
@bnoordhuis bnoordhuis merged commit d1f7c96 into nodejs:master Oct 24, 2016
evanlucas pushed a commit that referenced this pull request Nov 2, 2016
@bnoordhuis @evanlucas
test: move flaky test to test/pummel
4be1ba5 
Move sequential/test-crypto-timing-safe-equal-benchmarks to test/pummel
because it fails for me locally quite frequently and because it takes
about five or six seconds to complete, which is too long for a test in
test/sequential.

Fixes: #8744
PR-URL: #9241
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
evanlucas pushed a commit that referenced this pull request Nov 2, 2016
@bnoordhuis @evanlucas
test: make flaky pummel test more reliable
2044619 
Increase the number of iterations from 1e4 to 1e5.  Makes the test pass
for me locally when previously it would fail 9 out of 10 times because
the running time was not enough to smooth away the outliers.

Fixes: #8744
PR-URL: #9241
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
@evanlucas evanlucas mentioned this pull request Nov 3, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
@bnoordhuis
test: move flaky test to test/pummel
a5e0466 
Move sequential/test-crypto-timing-safe-equal-benchmarks to test/pummel
because it fails for me locally quite frequently and because it takes
about five or six seconds to complete, which is too long for a test in
test/sequential.

Fixes: #8744
PR-URL: #9241
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
@bnoordhuis
test: make flaky pummel test more reliable
7647777 
Increase the number of iterations from 1e4 to 1e5.  Makes the test pass
for me locally when previously it would fail 9 out of 10 times because
the running time was not enough to smooth away the outliers.

Fixes: #8744
PR-URL: #9241
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
MylesBorins pushed a commit that referenced this pull request Nov 19, 2016
@bnoordhuis @MylesBorins
test: move flaky test to test/pummel
a46c027 
Move sequential/test-crypto-timing-safe-equal-benchmarks to test/pummel
because it fails for me locally quite frequently and because it takes
about five or six seconds to complete, which is too long for a test in
test/sequential.

Fixes: #8744
PR-URL: #9241
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
MylesBorins pushed a commit that referenced this pull request Nov 19, 2016
@bnoordhuis @MylesBorins
test: make flaky pummel test more reliable
18a235b 
Increase the number of iterations from 1e4 to 1e5.  Makes the test pass
for me locally when previously it would fail 9 out of 10 times because
the running time was not enough to smooth away the outliers.

Fixes: #8744
PR-URL: #9241
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Merged
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
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.
7 participants
@bnoordhuis @not-an-aardvark @jasnell @cjihrig @mscdex @MylesBorins @nodejs-github-bot