Skip to content

test: reduce the allocation size in test-worker-arraybuffer-zerofill#54839

Closed
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:cleanup-test-worker-arraybuffer-zerofill
Closed

test: reduce the allocation size in test-worker-arraybuffer-zerofill#54839
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:cleanup-test-worker-arraybuffer-zerofill

Conversation

@jasnell

@jasnell jasnell commented Sep 7, 2024

Copy link
Copy Markdown
Member

Test has been flaky with timeouts in CI. This is possibly due to the repeated large allocations on the main thread. This commit reduces the allocation size and makes a number of other cleanups. The main goal is to hopefully make this test more reliable / not-flaky.

Also move the test to sequential. The frequent large allocations could be causing the test to be flaky if run parallel to other tests.

Refs: #52274

update: I realized after that it wasn't clear what was meant by "reduce the allocation size" here. The actual allocation on each iteration remains the same, but by waiting to start allocating until the worker reports the "online" event we reduce (albeit slightly) the number of allocations that are done overall. In local testing this reduced the actually number of allocation calls by a fair amount, reducing the amount of memory the test was using. Unfortunately I don't think just doing that is enough to moving the test to sequential should hopefully also help.

@jasnell jasnell requested review from anonrig and mcollina September 7, 2024 19:34
@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
@nodejs-github-bot

This comment was marked as outdated.

Comment thread test/sequential/test-worker-arraybuffer-zerofill.js
Comment thread test/sequential/test-worker-arraybuffer-zerofill.js
Comment thread test/sequential/test-worker-arraybuffer-zerofill.js
Comment thread test/sequential/test-worker-arraybuffer-zerofill.js
@lpinca

lpinca commented Sep 7, 2024

Copy link
Copy Markdown
Member

As written elsewhere I really see no reason to refactor this kind of tests to use node:test. In my opinion it is only added complexity and additional code that might interfere with what should be actually tested. The original design of using only the code strictly required for the test was great. There is no better way to write and run tests in my opinion. That said, I'm not blocking.

@codecov

codecov Bot commented Sep 7, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.06%. Comparing base (123bb4c) to head (49d5078).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54839      +/-   ##
==========================================
- Coverage   88.07%   88.06%   -0.01%     
==========================================
  Files         651      651              
  Lines      183396   183396              
  Branches    35795    35798       +3     
==========================================
- Hits       161523   161515       -8     
+ Misses      15161    15159       -2     
- Partials     6712     6722      +10     

see 29 files with indirect coverage changes

@jasnell

jasnell commented Sep 8, 2024

Copy link
Copy Markdown
Member Author

The stress test looks good.

@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell force-pushed the cleanup-test-worker-arraybuffer-zerofill branch from 4db976a to 066d836 Compare September 8, 2024 17:52
@nodejs-github-bot

This comment was marked as outdated.

@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
Test has been flaky with timeouts in CI. This is possibly due to the
repeated large allocations on the main thread. This commit reduces the
allocation size and makes a number of other cleanups. The main goal
is to hopefully make this test more reliable / not-flaky.

Also move the test to sequential. The frequent large allocations
could be causing the test to be flaky if run parallel to other tests.
@jasnell jasnell force-pushed the cleanup-test-worker-arraybuffer-zerofill branch from 066d836 to 49d5078 Compare September 11, 2024 14:39
@nodejs-github-bot

nodejs-github-bot commented Sep 11, 2024

Copy link
Copy Markdown
Collaborator

@mcollina mcollina 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

@jasnell

jasnell commented Sep 12, 2024

Copy link
Copy Markdown
Member Author

Landed in 4b2cab2

@jasnell jasnell closed this Sep 12, 2024
@lpinca

lpinca commented Sep 12, 2024

Copy link
Copy Markdown
Member

Some tests time out due to a deadlock during process shutdown, see #52550 (comment). I think that the underlying issue is the same here. We could revert/revisit this when a fix lands.

@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

6 participants