test: make test-perf-hooks more robust and work with workers#49197
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.
nodejs-github-bot added needs-ci 
MemberAuthor
joyeecheung commented Aug 16, 2023
From nodejs/reliability#638 this test has been failing 6 recent PRs, hopefully this makes the flake go away..
Example |
github-actionsbot removed the request-ci 
Collaborator
nodejs-github-bot commented Aug 16, 2023
Uh oh!
There was an error while loading. Please reload this page.
debadree25 added the author ready 
Member
debadree25 commented Aug 16, 2023
i tried stress testing this too locally with |
MemberAuthor
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). |
Member
debadree25 commented Aug 16, 2023
Yeah makes sense! |
aduh95 approved these changes Aug 16, 2023
| // 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`); |
Contributor
There was a problem hiding this comment.
nit
Suggested change
| assert(testStartTime<15000,`${testStartTime}>= 15000`); | |
| assert(testStartTime<15000,`${testStartTime}≥ 15000`); |
MemberAuthor
There was a problem hiding this comment.
I think in general we use >= in this codebase instead?
joyeecheung added the commit-queue MemberAuthor
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/ |
nodejs-github-bot removed the commit-queue Collaborator
nodejs-github-bot commented Aug 18, 2023
Landed in ecde9d9 |
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Merged
targos pushed a commit that referenced this pull request Nov 27, 2023
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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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