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: reduce stack size for test-error-serdes #54840

Closed
wants to merge 3 commits into from
Closed

test: reduce stack size for test-error-serdes #54840

wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 7, 2024

Hopefully reduces the run time and the likelihood of the test failing with a flaky timeout error.

Refs: #52630

@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 7, 2024
@jasnell
Copy link
Member Author

jasnell commented Sep 7, 2024

Stress test: https://ci.nodejs.org/view/Stress/job/node-stress-single-test/532/

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@codecov Codecov
Copy link

codecov bot commented Sep 7, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.89%. Comparing base (e1e312d) to head (4001157).
Report is 172 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54840      +/-   ##
==========================================
- Coverage   87.90%   87.89%   -0.01%     
==========================================
  Files         651      651              
  Lines      183343   183343              
  Branches    35710    35712       +2     
==========================================
- Hits       161165   161155      -10     
+ Misses      15466    15461       -5     
- Partials     6712     6727      +15

see 32 files with indirect coverage changes

@LiviaMedeiros
Copy link
Contributor

IIRC this made test more flaky on armv7 (32bit), do we still have these in stress CI?

@jasnell
Copy link
Member Author

jasnell commented Sep 8, 2024 •
edited
Loading

@LiviaMedeiros .... can you expand on that a bit more? Maybe a link to prior attempts/discussion

Stress test covers:
Uploading image.png…

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 8, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@LiviaMedeiros
Copy link
Contributor

can you expand on that a bit more? Maybe a link to prior attempts/discussion

b3d0804 (#52674) was an attempt to do the same. The source of flakiness was confirmed to be code using ErrorWithCyclicCause and the change indeed speeds up unwinding, however limiting stack size haven't solved the issue (basically, when the bug appears and test is not killed by timeout, it stayed in loop for 20+ minutes regardless of stack limit) and for whatever reason made it flake on armv7 even more frequently.
I don't see an option to start stress test CI on any 32bit arch though.

@jasnell
Copy link
Member Author

jasnell commented Sep 8, 2024

@LiviaMedeiros ... good fun.. ok. Did anyone try moving the test to the sequential suite rather than the parallel suite? Maybe there's just too much going on concurrently

@LiviaMedeiros
Copy link
Contributor

The flake was reproducible locally (although not as frequent as in CI) using sequential for i in $(seq 9999); do echo $i; node test.js; done, but it's worth a try.

@jasnell jasnell force-pushed the limit-stack-size-error-serdes branch from ad383b8 to 4001157 Compare September 8, 2024 18:09
@jasnell
Copy link
Member Author

jasnell commented Sep 8, 2024

@LiviaMedeiros ... done! Moved the test over to sequential with the lower stack limit. Let's give this a try and see if it at least reduces how often it flakes.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as duplicate.

Sign in to view
@jasnell
Copy link
Member Author

jasnell commented Sep 8, 2024

heh, we kicked off CI at the same time lol.

@LiviaMedeiros
Copy link
Contributor

Almost cancelled at the same time as well 😄
Stress CI just in case: https://ci.nodejs.org/job/node-stress-single-test/534/

@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
@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
aduh95
aduh95 approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

🎉

@nodejs-github-bot
Copy link
Collaborator

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

jasnell added a commit that referenced this pull request Sep 11, 2024
@jasnell
test: reduce stack size for test-error-serdes
3783499 
Hopefully reduces the run time and the likelihood of the test
failing with a flaky timeout error.

remove test-error-serdes from flaky list

move test-error-serdes to sequential

PR-URL: #54840
Refs: #52630
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Sep 11, 2024

Landed in 3783499

@jasnell jasnell closed this Sep 11, 2024
aduh95 pushed a commit that referenced this pull request Sep 12, 2024
@jasnell @aduh95
test: reduce stack size for test-error-serdes
784189b 
Hopefully reduces the run time and the likelihood of the test
failing with a flaky timeout error.

remove test-error-serdes from flaky list

move test-error-serdes to sequential

PR-URL: #54840
Refs: #52630
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
aduh95 pushed a commit that referenced this pull request Sep 13, 2024
@jasnell @aduh95
test: reduce stack size for test-error-serdes
8eaea0c 
Hopefully reduces the run time and the likelihood of the test
failing with a flaky timeout error.

remove test-error-serdes from flaky list

move test-error-serdes to sequential

PR-URL: #54840
Refs: #52630
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
aduh95 pushed a commit that referenced this pull request Sep 13, 2024
@jasnell @aduh95
test: reduce stack size for test-error-serdes
b875f2d 
Hopefully reduces the run time and the likelihood of the test
failing with a flaky timeout error.

remove test-error-serdes from flaky list

move test-error-serdes to sequential

PR-URL: #54840
Refs: #52630
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 22, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@jasnell @louwers
test: reduce stack size for test-error-serdes
24bc910 
Hopefully reduces the run time and the likelihood of the test
failing with a flaky timeout error.

remove test-error-serdes from flaky list

move test-error-serdes to sequential

PR-URL: nodejs#54840
Refs: nodejs#52630
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@jasnell @tpoisseau
test: reduce stack size for test-error-serdes
e3431ce 
Hopefully reduces the run time and the likelihood of the test
failing with a flaky timeout error.

remove test-error-serdes from flaky list

move test-error-serdes to sequential

PR-URL: nodejs#54840
Refs: nodejs#52630
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LiviaMedeiros LiviaMedeiros LiviaMedeiros approved these changes

@trivikr trivikr trivikr approved these changes

@aduh95 aduh95 aduh95 approved these changes

@MoLow MoLow MoLow approved these changes

@anonrig anonrig anonrig approved these changes

@targos targos Awaiting requested review from targos

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. 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.
8 participants
@jasnell @nodejs-github-bot @LiviaMedeiros @anonrig @MoLow @aduh95 @trivikr @targos