Skip to content

test: deflake test-buffer-large-size - #57789

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
jakecastelli:deflake-test-buffer-large-size
Apr 11, 2025
Merged

test: deflake test-buffer-large-size#57789
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
jakecastelli:deflake-test-buffer-large-size

Conversation

@jakecastelli

@jakecastellijakecastelli commented Apr 8, 2025•
edited
Loading

Copy link
Copy Markdown
Member

The test has failed 25+ times on 7th of April and 24 times 8th of April in our CI.

This PR attempts to spread the tests into multiple file to reduce the flakiness.

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Apr 8, 2025
@jakecastelli

jakecastelli commented Apr 8, 2025

Copy link
Copy Markdown
MemberAuthor

queued a stress test - https://ci.nodejs.org/job/node-stress-single-test/557/

lpinca
lpinca reviewed Apr 8, 2025
View reviewed changes
Comment threadtest/pummel/test-buffer-large-size.js Outdated
@lpinca

lpinca commented Apr 8, 2025

Copy link
Copy Markdown
Member

I'm not sure I understand the difference. Why using a for loop reduces the flakiness?

@jakecastelli

jakecastelli commented Apr 8, 2025

Copy link
Copy Markdown
MemberAuthor

Currently it is only one test that attempts to allocate 8GB of memory, the purpose of the for loop is to make them into 4 separate tests. I think it could have 2 benefits:

  1. we could be able to to know which one timed out
  2. potentially give more chances to gc

@jakecastelli

jakecastelli commented Apr 8, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Actually - now I see my stress tests start to fail on ubuntu2204-arm64 with the same timeout, I think you might be right @lpinca 🤔 it wouldn't do anything useful, do you think this also relates to the deadlock? Maybe run a major gc is a better way to deflake the test 😅 wdyt?

@lpinca

lpinca commented Apr 8, 2025

Copy link
Copy Markdown
Member

Do you think this also relates to the deadlock?

Yes, probably.

@jakecastelli
jakecastelliforce-pushed the deflake-test-buffer-large-size branch from 58ebe5a to 5fae384CompareApril 8, 2025 13:43
@jakecastelli

jakecastelli commented Apr 8, 2025

Copy link
Copy Markdown
MemberAuthor

I've attempted to add major gc after each allocation and queued another stress test - https://ci.nodejs.org/job/node-stress-single-test/558/. This test seems having a way too high failure rate.

@RaisinTen

RaisinTen commented Apr 8, 2025

Copy link
Copy Markdown
Member

Might be worth checking if distributing each of these into separate test files improves things

@nodejs-github-bot

nodejs-github-bot commented Apr 8, 2025

Copy link
Copy Markdown
Collaborator

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

@jakecastelli

jakecastelli commented Apr 9, 2025

Copy link
Copy Markdown
MemberAuthor

The stress tests seem ok, the CI failures relate to other flaky tests. Are we happy to do major gc or better off separate them into different files?

@jakecastelli
jakecastelliforce-pushed the deflake-test-buffer-large-size branch from 5fae384 to 4e19317CompareApril 9, 2025 12:12
@jakecastelli

jakecastelli commented Apr 9, 2025

Copy link
Copy Markdown
MemberAuthor

Any further actionable item should I take?

  • Continue with landing this PR
  • Separate the tests into multiple files

Happy to take either path 👍

@lpinca

lpinca commented Apr 9, 2025

Copy link
Copy Markdown
Member

I would try to separate into multiple files without manually triggering GC and see what happens. Manually calling the GC is not a fix.

@jakecastelli
jakecastelliforce-pushed the deflake-test-buffer-large-size branch from 4e19317 to 920046fCompareApril 9, 2025 12:34
@jakecastelli
jakecastelliforce-pushed the deflake-test-buffer-large-size branch from 920046f to e6b2341CompareApril 9, 2025 12:38
@jakecastelli
jakecastelli marked this pull request as ready for review April 9, 2025 12:46
@jakecastelli

jakecastelli commented Apr 9, 2025

Copy link
Copy Markdown
MemberAuthor

what are the practical ways to verify if things've improved?

  • have a few CI runs
  • 1000 stress tests

Any other suggestions?

RaisinTen
RaisinTen approved these changes Apr 9, 2025
View reviewed changes

@RaisinTenRaisinTen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM
Added some optional suggestions to simplify the tests

Comment threadtest/pummel/test-buffer-large-size-buffer-alloc-unsafe-slow.js Outdated
Comment threadtest/pummel/test-buffer-large-size-buffer-alloc-unsafe-slow.js Outdated
@codecov

codecovBot commented Apr 9, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (1540fc6) to head (c76d93b).
Report is 242 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57789 +/- ##
==========================================
- Coverage 90.23% 90.22% -0.02% 
==========================================
Files 630 630 Lines 185288 185518 +230 Branches 36344 36380 +36 ==========================================
+ Hits 167203 167387 +184 - Misses 11006 11027 +21 - Partials 7079 7104 +25

see 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 9, 2025

Copy link
Copy Markdown
Collaborator

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

RaisinTen
RaisinTen approved these changes Apr 10, 2025
View reviewed changes

@RaisinTenRaisinTen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks

@jakecastellijakecastelli added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 10, 2025
@jakecastellijakecastelli added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 10, 2025
@jakecastellijakecastelli added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 11, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 11, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 795dd8e into nodejs:mainApr 11, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 11, 2025

Copy link
Copy Markdown
Collaborator

Landed in 795dd8e

@sapphi-redsapphi-red mentioned this pull request Apr 15, 2025
Open
@RaisinTenRaisinTen mentioned this pull request Apr 25, 2025
Closed
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@jakecastelli@RafaelGSS
test: deflake test-buffer-large-size
bbb50e1
PR-URL: #57789
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@jakecastelli@RafaelGSS
test: deflake test-buffer-large-size
660d238
PR-URL: #57789
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@jakecastelli@aduh95
test: deflake test-buffer-large-size
bc1e7f0
PR-URL: #57789
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label May 6, 2025
@aduh95

aduh95 commented May 6, 2025

Copy link
Copy Markdown
Contributor

Tests are failing on GHA with this change (https://github.com/nodejs/node/actions/runs/14865929007/job/41742567775), we'd need a manual backport PR if we want to port this change.

@jakecastelli

jakecastelli commented May 7, 2025

Copy link
Copy Markdown
MemberAuthor

I will look into the backport

@jakecastelli

jakecastelli commented May 8, 2025

Copy link
Copy Markdown
MemberAuthor

Hi @aduh95 I took a look and realised this PR was fixing the flaky test introduced in #51821 which 51821 is a semver-majorPRs that contain breaking changes and should be released in the next major version. so, I think I should've added don't land on labels

@jakecastellijakecastelli added dont-land-on-v20.x dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. and removed backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels May 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes
@RaisinTenRaisinTenRaisinTen approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.dont-land-on-v22.xPRs that should not land on the v22.x-staging branch and should not be released in v22.x.needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jakecastelli@lpinca@RaisinTen@nodejs-github-bot@aduh95@jasnell