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 node-api/test_buffer/test_finalizer not flaky #43418

Closed
wants to merge 2 commits into from
Closed

test: make node-api/test_buffer/test_finalizer not flaky #43418

wants to merge 2 commits into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Jun 14, 2022 •
edited
Loading

Trying to improve the reliability of test/node-api/test_buffer/test_finalizer.js as it is very often failing.

@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 Jun 14, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
legendecas
legendecas approved these changes Jun 14, 2022
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix! I was figuring out why the test is unstable on AIX but I don't have access to such an environment locally.

@mcollina
Copy link
Member Author

I don't have a failing environment either, however common.platformTimeout() is a best practice and a good solution for timing and gc issues. It's worth a try.

@benjamingr benjamingr added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 14, 2022
@github-actions
Copy link
Contributor

Fast-track has been requested by @benjamingr. Please 👍 to approve.

@benjamingr
Copy link
Member

requesting fast-track since this is a test fix for ci

@tniessen
Copy link
Member

Stress test on AIX: https://ci.nodejs.org/job/node-stress-single-test/343/
Master: https://ci.nodejs.org/job/node-stress-single-test/344/

@tniessen
Copy link
Member

tniessen commented Jun 14, 2022 •
edited
Loading

@benjamingr #43414 is ready to land and already being fast-tracked if we want to fix CI quickly.

Edit: Sorry, looks like no CI ran on either PR yet.

@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen
Copy link
Member

First stress test still shows 9 % failures with this patch.

@benjamingr benjamingr removed the fast-track PRs that do not need to wait for 48 hours to land. label Jun 14, 2022
@mcollina
Copy link
Member Author

It's still the right fix to do.

@mcollina
Copy link
Member Author

Funny bit is that this passed on Aix :).

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@mhdawson
Copy link
Member

Sorry just noticed this and already landed my earlier change to mark the test flaky. When ready this PR should remove from being flaky as well.

@richardlau
Copy link
Member

Sorry just noticed this and already landed my earlier change to mark the test flaky. When ready this PR should remove from being flaky as well.

Unfortunately the test is still flaky with this patch, #43418 (comment).

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@mcollina
Copy link
Member Author

First stress test still shows 9 % failures with this patch.

Why are you saying so? all the the stress tests in CI are failing for wrong parameters.

@tniessen
Copy link
Member

@mcollina I don't know what happened on the aix71 machines, but please look at aix72:

09:46:44 Cloning repository [email protected]:mcollina/node.git
...
09:50:24 Checking out Revision fe3d9c25b2670790a0753070d6ef10d9eeda2099
...
09:50:29 Commit message: "test: use platformTimeout for node-api/test_buffer/test_finalizer"
...
10:29:39 + echo '1000   OK: 910   NOT OK: 90   TOTAL: 1000'
10:29:39 1000   OK: 910   NOT OK: 90   TOTAL: 1000

@github-actions github-actions bot mentioned this pull request Jun 15, 2022
Open
17 tasks
@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 15, 2022
@mcollina
Copy link
Member Author

Feel free to ping if there are others that are GC related, or apply the same "trick" I used here.

@github-actions github-actions bot mentioned this pull request Jun 16, 2022
Open
22 tasks
mhdawson
mhdawson approved these changes Jun 16, 2022
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for investigating/fixing

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 17, 2022
@github-actions github-actions bot mentioned this pull request Jun 17, 2022
Open
31 tasks
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 17, 2022
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 17, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 17, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 7fd4cf4...562bf8b

nodejs-github-bot pushed a commit that referenced this pull request Jun 17, 2022
@mcollina @nodejs-github-bot
test: make node-api/test_buffer/test_finalizer not flaky
8bff086 
PR-URL: #43418
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Jun 17, 2022
@mcollina @nodejs-github-bot
Revert "test: mark test_buffer/test_finalizer flaky"
562bf8b 
This reverts commit 73d8db8.

PR-URL: #43418
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson mhdawson mentioned this pull request Jun 17, 2022
Closed
@github-actions github-actions bot mentioned this pull request Jun 18, 2022
Open
34 tasks
targos pushed a commit that referenced this pull request Jul 12, 2022
@mcollina @targos
test: make node-api/test_buffer/test_finalizer not flaky
3948acc 
PR-URL: #43418
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
@mcollina @targos
Revert "test: mark test_buffer/test_finalizer flaky"
2b55b60 
This reverts commit 73d8db8.

PR-URL: #43418
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 18, 2022
@mcollina @targos
test: make node-api/test_buffer/test_finalizer not flaky
c93c680 
PR-URL: #43418
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jul 18, 2022
@mcollina @targos
Revert "test: mark test_buffer/test_finalizer flaky"
27eb409 
This reverts commit 73d8db8.

PR-URL: #43418
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@mcollina @targos
test: make node-api/test_buffer/test_finalizer not flaky
3833640 
PR-URL: #43418
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@mcollina @targos
Revert "test: mark test_buffer/test_finalizer flaky"
6fcb349 
This reverts commit 73d8db8.

PR-URL: #43418
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@mcollina @guangwong
test: make node-api/test_buffer/test_finalizer not flaky
b523d1b 
PR-URL: nodejs/node#43418
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@mcollina @guangwong
Revert "test: mark test_buffer/test_finalizer flaky"
fbd91de 
This reverts commit 73d8db8.

PR-URL: nodejs/node#43418
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@legendecas legendecas legendecas approved these changes

@richardlau richardlau richardlau approved these changes

@tniessen tniessen tniessen approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mcollina @nodejs-github-bot @benjamingr @tniessen @mhdawson @richardlau @jasnell @legendecas @aduh95