test: make node-api/test_buffer/test_finalizer not flaky by mcollina · Pull Request #43418 · nodejs/node

mcollina · 2022-06-14T07:00:51Z

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

legendecas

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 · 2022-06-14T07:30:41Z

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.

github-actions · 2022-06-14T07:44:15Z

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

benjamingr · 2022-06-14T07:44:18Z

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

tniessen · 2022-06-14T07:45:21Z

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 · 2022-06-14T07:46:21Z

@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 · 2022-06-14T08:45:09Z

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

mcollina · 2022-06-14T09:05:17Z

It's still the right fix to do.

mcollina · 2022-06-14T09:42:41Z

Funny bit is that this passed on Aix :).

mhdawson · 2022-06-14T14:51:10Z

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 · 2022-06-14T15:43:12Z

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).

mcollina · 2022-06-14T21:38:55Z

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 · 2022-06-14T22:13:36Z

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

09:46:44 Cloning repository git@github.com: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

mcollina · 2022-06-15T23:02:18Z

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

mhdawson

LGTM, thanks for investigating/fixing

nodejs-github-bot · 2022-06-16T22:44:46Z

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

nodejs-github-bot · 2022-06-17T08:32:59Z

Landed in 7fd4cf4...562bf8b

PR-URL: #43418 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

This reverts commit 73d8db8. PR-URL: #43418 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: #43418 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

This reverts commit 73d8db8. PR-URL: #43418 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: #43418 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

This reverts commit 73d8db8. PR-URL: #43418 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: #43418 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

This reverts commit 73d8db8. PR-URL: #43418 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: nodejs/node#43418 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

This reverts commit 73d8db8. PR-URL: nodejs/node#43418 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

mcollina requested review from legendecas and mhdawson June 14, 2022 07:00

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

This comment was marked as outdated.

Sign in to view

benjamingr approved these changes Jun 14, 2022

View reviewed changes

legendecas approved these changes Jun 14, 2022

View reviewed changes

tniessen approved these changes Jun 14, 2022

View reviewed changes

benjamingr added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 14, 2022

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2022

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

This comment was marked as outdated.

Sign in to view

benjamingr removed the fast-track PRs that do not need to wait for 48 hours to land. label Jun 14, 2022

This comment was marked as outdated.

Sign in to view

jasnell approved these changes Jun 14, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Jun 15, 2022

CI Reliability 2022-06-15 nodejs/reliability#301

Open

17 tasks

richardlau approved these changes Jun 15, 2022

View reviewed changes

github-actions bot mentioned this pull request Jun 16, 2022

CI Reliability 2022-06-16 nodejs/reliability#302

Open

22 tasks

mhdawson approved these changes Jun 16, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 17, 2022

github-actions bot mentioned this pull request Jun 17, 2022

CI Reliability 2022-06-17 nodejs/reliability#303

Open

31 tasks

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 17, 2022

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 17, 2022

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 closed this Jun 17, 2022

mhdawson mentioned this pull request Jun 17, 2022

Investigate flaky test - node-api/test_buffer/test_finalizer #43389

Closed

github-actions bot mentioned this pull request Jun 18, 2022

CI Reliability 2022-06-18 nodejs/reliability#304

Open

34 tasks

targos mentioned this pull request Jul 12, 2022

v18.6.0 release proposal #43789

Merged

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Uh oh!

Conversation

mcollina commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

legendecas left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jun 14, 2022

Uh oh!

github-actions bot commented Jun 14, 2022

Uh oh!

benjamingr commented Jun 14, 2022

Uh oh!

tniessen commented Jun 14, 2022

Uh oh!

tniessen commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

tniessen commented Jun 14, 2022

Uh oh!

mcollina commented Jun 14, 2022

Uh oh!

mcollina commented Jun 14, 2022

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

mhdawson commented Jun 14, 2022

Uh oh!

richardlau commented Jun 14, 2022

Uh oh!

This comment was marked as outdated.

mcollina commented Jun 14, 2022

Uh oh!

tniessen commented Jun 14, 2022

Uh oh!

mcollina commented Jun 15, 2022

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Jun 16, 2022

Uh oh!

nodejs-github-bot commented Jun 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mcollina commented Jun 14, 2022 •

edited

Loading

tniessen commented Jun 14, 2022 •

edited

Loading