Skip to content

Rewrite fastutf8stream tests node test#59667

Closed
mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina:rewrite-fastutf8stream-tests-node-test
Closed

Rewrite fastutf8stream tests node test#59667
mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina:rewrite-fastutf8stream-tests-node-test

Conversation

@mcollina
Copy link
Copy Markdown
Member

@mcollina mcollina commented Aug 28, 2025

Fixes #59638

@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 Aug 28, 2025
@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Aug 28, 2025

@joyeecheung I would need an approval to be able to test it on CI

jasnell
jasnell reviewed Aug 28, 2025
View reviewed changes
Comment thread test/parallel/test-fastutf8stream-flush-mocks.js
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 28, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.91%. Comparing base (ebd2da6) to head (030514c).
⚠️ Report is 31 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #59667      +/-   ##
==========================================
+ Coverage   89.90%   89.91%   +0.01%     
==========================================
  Files         667      667              
  Lines      196601   196600       -1     
  Branches    38599    38607       +8     
==========================================
+ Hits       176757   176782      +25     
+ Misses      12327    12267      -60     
- Partials     7517     7551      +34

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

@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Aug 29, 2025 •
edited
Loading

Stress CI: https://ci.nodejs.org/job/node-stress-single-test/613/

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2025
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 29, 2025

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

@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Aug 29, 2025

Screenshot 2025-08-29 at 10 00 45

@nodejs/build I'm having a bit of hard time in figuring out how to start the stress test CI

@mcollina
test: rewrite fastutf8stream tests to use node:test
030514c 
Rewrite test/parallel/test-fastutf8stream-sync.js and
test/parallel/test-fastutf8stream-flush-mocks.js to use node:test
with individual test functions and proper stream cleanup.

Key changes:
- Convert from block-based tests to individual test() functions
- Use once() from node:events for proper stream close event handling
- Ensure streams emit 'close' event before test completion for Windows
  compatibility
- Fix fsync mocking in flush tests to allow proper stream closure
- Each test runs independently with proper resource cleanup
@mcollina mcollina force-pushed the rewrite-fastutf8stream-tests-node-test branch from 13a64e6 to 030514c Compare August 29, 2025 11:51
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 31, 2025

Failed to start CI
   ⚠  Commits were pushed since the last approving review:
   ⚠  - test: rewrite fastutf8stream tests to use node:test
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/17357709189

@github-actions github-actions Bot added the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Aug 31, 2025
@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Sep 1, 2025

Ok it seems that the stress test CI now started: https://ci.nodejs.org/view/Stress/job/node-stress-single-test/615/.

I had to pick win2022-vs2022_clang as a configuration.

@joyeecheung
Copy link
Copy Markdown
Member

joyeecheung commented Sep 1, 2025

FWIW the original flakes are now gone in the reliability report https://github.com/nodejs/reliability/blob/main/reports/2025-09-01.md

@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Sep 1, 2025

The problem is still there, it really depends on the drive of those virtual machines.

@lpinca
Copy link
Copy Markdown
Member

lpinca commented Sep 1, 2025

I think that splitting the test cases into different files would be better for readability, debuggability, proper parallelization, etc.

@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Sep 1, 2025

Considering that those tests are no longer a problem, it's probably not worth the effort.

@mcollina mcollina closed this Sep 1, 2025
@mcollina mcollina mentioned this pull request Sep 1, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

parallel/test-fastutf8stream-flush-mocks and parallel/test-fastutf8stream-sync are flaky on Windows

5 participants

@mcollina @nodejs-github-bot @joyeecheung @lpinca @jasnell