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.

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: deflake test-perf-hooks.js #49892

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Sep 27, 2023

Previously when checking the initial timing we did a lot of checks after accessing and copying timing.duration and before we check that timing.duration is roughly the same as performance.now(), which can lead to flakes if the overhead of the checking is big enough. Update the test to check timing.duration against performance.now() as soon as possible when it's copied instead of computed.

Refs: nodejs/reliability#676

Previously when checking the initial timing we did a lot of checks
after accessing and copying timing.duration and before we check
that timing.duration is roughly the same as performance.now(),
which can lead to flakes if the overhead of the checking is
big enough. Update the test to check timing.duration against
performance.now() as soon as possible when it's copied instead
of computed.
:#
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 27, 2023
@joyeecheung
Copy link
Member Author

We don't have shared library boxes for the stress tests so I only started the stress test on osx-11

PR: https://ci.nodejs.org/job/node-stress-single-test/450/
main: https://ci.nodejs.org/job/node-stress-single-test/451/

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 29, 2023
joyeecheung added a commit that referenced this pull request Sep 29, 2023
Previously when checking the initial timing we did a lot of checks
after accessing and copying timing.duration and before we check
that timing.duration is roughly the same as performance.now(),
which can lead to flakes if the overhead of the checking is
big enough. Update the test to check timing.duration against
performance.now() as soon as possible when it's copied instead
of computed.
:#

PR-URL: #49892
Refs: nodejs/reliability#676
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Richard Lau <rlau@redhat.com>
@joyeecheung
Copy link
Member Author

joyeecheung commented Sep 29, 2023

CI was green but the bot had trouble communicating the CI status back to GitHub. Landed manually in 4d0aeed

@joyeecheung joyeecheung removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 29, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
Previously when checking the initial timing we did a lot of checks
after accessing and copying timing.duration and before we check
that timing.duration is roughly the same as performance.now(),
which can lead to flakes if the overhead of the checking is
big enough. Update the test to check timing.duration against
performance.now() as soon as possible when it's copied instead
of computed.
:#

PR-URL: nodejs#49892
Refs: nodejs/reliability#676
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
Previously when checking the initial timing we did a lot of checks
after accessing and copying timing.duration and before we check
that timing.duration is roughly the same as performance.now(),
which can lead to flakes if the overhead of the checking is
big enough. Update the test to check timing.duration against
performance.now() as soon as possible when it's copied instead
of computed.
:#

PR-URL: #49892
Refs: nodejs/reliability#676
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Richard Lau <rlau@redhat.com>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
Previously when checking the initial timing we did a lot of checks
after accessing and copying timing.duration and before we check
that timing.duration is roughly the same as performance.now(),
which can lead to flakes if the overhead of the checking is
big enough. Update the test to check timing.duration against
performance.now() as soon as possible when it's copied instead
of computed.
:#

PR-URL: nodejs#49892
Refs: nodejs/reliability#676
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Richard Lau <rlau@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants