Skip to content

test: fix flaky HTTP/2 reset callback check#64730

Open
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:test-http2-reset-happy-close
Open

test: fix flaky HTTP/2 reset callback check#64730
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:test-http2-reset-happy-close

Conversation

@trivikr

@trivikr trivikr commented Jul 25, 2026

Copy link
Copy Markdown
Member

Refs: https://github.com/nodejs/reliability/issues?q=sort%3Aupdated-desc%20test-http2-reset-happy-close

The test previously expected all pending write callbacks to run within two
setImmediate() turns after the HTTP/2 stream emitted 'close'. Write
callbacks can run later when data has already been handed to the socket,
making that timing assumption unreliable under CI load.

Track stream closure and write callback completion independently, and close
the server only after both conditions are satisfied. Use common.mustCall()
to continue verifying that all 16 callbacks run. If a callback is genuinely
lost, the test harness will time out or report the callback count mismatch.


Assisted-by: codex:gpt-5.6-sol

The test assumed that all pending write callbacks would run within two
event loop iterations after the stream closed. Callbacks for writes
already handed to the socket can run later, causing a flaky assertion.

Wait for the stream to close and all expected write callbacks to run
before closing the server.

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: codex:gpt-5.6-sol
@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 Jul 25, 2026
@trivikr

trivikr commented Jul 25, 2026

Copy link
Copy Markdown
Member Author

@trivikr
trivikr marked this pull request as ready for review July 25, 2026 21:39
@trivikr trivikr added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Jul 25, 2026
@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (9024119) to head (b8b5d04).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64730      +/-   ##
==========================================
- Coverage   90.15%   90.14%   -0.01%     
==========================================
  Files         743      743              
  Lines      242407   242407              
  Branches    45645    45659      +14     
==========================================
- Hits       218532   218527       -5     
- Misses      15357    15384      +27     
+ Partials     8518     8496      -22     

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

@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2026

@inoway46 inoway46 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I specifically checked the removal of the explicit writeCbCount assertion: common.mustCall(..., N_WRITES) and the maybeCloseServer() gate preserve the exact-count invariant, so this removes the timing assumption without weakening the test coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flaky-test Issues and PRs related to the tests with unstable failures on the CI. needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants