Skip to content

test: mark test-file-write-stream4 as flaky #57927

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 18, 2025

The flake has been detected since 2021-10-30 and still has been affecting recent PRs significantly (failing 8 PRs out of the last 100 CI run up until 2025-04-18). The underlying cause is still under investigation but in the meantime it's better to mark it as a known flake to keep the CI functional.

This flake affects at least Windows, Linux and macOS, according to https://github.com/nodejs/reliability/blob/main/reports/2025-04-18.md and likely affects all platforms. So the status is applied to all platforms in this PR.

Refs: nodejs/reliability#102
Refs: https://github.com/nodejs/reliability/blob/main/reports/2025-04-18.md
Refs: #56883
Refs: #54918

The flake has been detected since 2021-10-30 and still has been
affecting recent PRs significantly (failing 8 PRs out of the
last 100 CI run up until 2025-04-18). The underlying cause
is still under investigation but in the meantime it's better
to mark it as a known flake to keep the CI functional.
@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 Apr 18, 2025
@lpinca
Copy link
Member

lpinca commented Apr 18, 2025

If we have to do this, I would prefer to manually call the GC when the test finishes, and add a TODO comment to remove it when #54918 is fixed.

@lpinca
Copy link
Member

lpinca commented Apr 18, 2025

I mean something like this

diff --git a/test/parallel/test-file-write-stream4.js b/test/parallel/test-file-write-stream4.js
index 6b3862fa714..1aa538f7781 100644
--- a/test/parallel/test-file-write-stream4.js
+++ b/test/parallel/test-file-write-stream4.js
@@ -1,3 +1,4 @@
+// Flags: --expose-gc
 'use strict';
 
 // Test that 'close' emits once and not twice when `emitClose: true` is set.
@@ -17,4 +18,8 @@ const fileWriteStream = fs.createWriteStream(filepath, {
 });
 
 fileReadStream.pipe(fileWriteStream);
-fileWriteStream.on('close', common.mustCall());
+fileWriteStream.on('close', common.mustCall(() => {
+  // TODO(lpinca): Remove the forced GC when
+  // https://github.com/nodejs/node/issues/54918 is fixed.
+  globalThis.gc({type: 'major'});
+}));

The advantage is that the test is still testing what it is supposed to test.

Copy link

codecov bot commented Apr 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (8e4e4df) to head (488dde4).
Report is 15 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #57927    +/-   ##
========================================
  Coverage   90.24%   90.24%            
========================================
  Files         630      630            
  Lines      185705   185933   +228     
  Branches    36407    36450    +43     
========================================
+ Hits       167591   167803   +212     
+ Misses      11002    10985    -17     
- Partials     7112     7145    +33     

see 46 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.

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 18, 2025

I remember that @jakecastelli mentioned that doing GC on shutdown doesn't actually make the flake go away?

@lpinca
Copy link
Member

lpinca commented Apr 18, 2025

I remember that @jakecastelli mentioned that doing GC on shutdown doesn't actually make the flake go away?

I missed that. Can you find where?

lpinca added a commit to lpinca/node that referenced this pull request Apr 18, 2025
Reduce `test/parallel/test-file-write-stream4.js`.

Refs: nodejs#57927
lpinca added a commit to lpinca/node that referenced this pull request Apr 18, 2025
Reduce `test/parallel/test-file-write-stream4.js`.

Refs: nodejs#57927
lpinca added a commit to lpinca/node that referenced this pull request Apr 18, 2025
Reduce `test/parallel/test-file-write-stream4.js` flakyness.

Refs: nodejs#57927
lpinca added a commit to lpinca/node that referenced this pull request Apr 19, 2025
Reduce `test/parallel/test-file-write-stream4.js` flakiness.

Refs: nodejs#57927
@jakecastelli
Copy link
Member

Hi folks, I think @joyeecheung meant a private slack direct message where I reached out to her when I found the manual gc from cc side didn't resolve the flakness of the test. But it was my fault where I triggered the gc at the wrong spot.

Later on I came up with this solution but we decided to not go with it. Hopefully this helps @lpinca

nodejs-github-bot pushed a commit that referenced this pull request Apr 20, 2025
Reduce `test/parallel/test-file-write-stream4.js` flakiness.

Refs: #57927
PR-URL: #57930
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants