-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: make test-perf-hooks more robust and work with workers #49197
Conversation
Previously the test makes several assumptions about the absolute values of the nodeTiming fields, which can make the test flaky on slow machines. This patch rewrites the test to check the relative values instead. It also updates the test to make it work with workers instead of directly skipping in workers.
From nodejs/reliability#638 this test has been failing 6 recent PRs, hopefully this makes the flake go away..
Example
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Thank you!
i tried stress testing this too locally with |
I am not sure why it was in sequential in the first place, but I think on the safe side we can just keep it there in this PR and if it looks well in the CI, move it to parallel afterwards (otherwise one doesn't know which is causing a regression if there is a regression). |
Yeah makes sense! |
// Use a fairly large epsilon value, since we can only guarantee that the node | ||
// process started up in 15 seconds. | ||
assert(Math.abs(performance.timeOrigin - Date.now()) < 15000); | ||
assert(testStartTime < 15000, `${testStartTime} >= 15000`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
assert(testStartTime < 15000, `${testStartTime} >= 15000`); | |
assert(testStartTime < 15000, `${testStartTime} ≥ 15000`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general we use >= in this codebase instead?
By the way the failure also shows up on v20.x-staging https://ci.nodejs.org/job/node-test-binary-armv7l/7027/RUN_SUBSET=js,nodes=ubuntu2004-armv7l/testReport/junit/(root)/sequential/test_perf_hooks/ |
Landed in ecde9d9 |
Previously the test makes several assumptions about the absolute values of the nodeTiming fields, which can make the test flaky on slow machines. This patch rewrites the test to check the relative values instead. It also updates the test to make it work with workers instead of directly skipping in workers. PR-URL: #49197 Refs: nodejs/reliability#638 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Previously the test makes several assumptions about the absolute values of the nodeTiming fields, which can make the test flaky on slow machines. This patch rewrites the test to check the relative values instead. It also updates the test to make it work with workers instead of directly skipping in workers. PR-URL: #49197 Refs: nodejs/reliability#638 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Previously the test makes several assumptions about the absolute values of the nodeTiming fields, which can make the test flaky on slow machines. This patch rewrites the test to check the relative values instead. It also updates the test to make it work with workers instead of directly skipping in workers. PR-URL: nodejs/node#49197 Refs: nodejs/reliability#638 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Previously the test makes several assumptions about the absolute values of the nodeTiming fields, which can make the test flaky on slow machines. This patch rewrites the test to check the relative values instead. It also updates the test to make it work with workers instead of directly skipping in workers. PR-URL: nodejs/node#49197 Refs: nodejs/reliability#638 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Previously the test makes several assumptions about the absolute values of the nodeTiming fields, which can make the test flaky on slow machines. This patch rewrites the test to check the relative values instead. It also updates the test to make it work with workers instead of directly skipping in workers.
Refs: nodejs/reliability#638