Skip to content

test(h2): cover stream reset with NGHTTP2_INTERNAL_ERROR - #5725

Open
zeexzeex wants to merge 1 commit into
nodejs:mainfrom
zeexzeex:h2-internal-error-test
Open

test(h2): cover stream reset with NGHTTP2_INTERNAL_ERROR#5725
zeexzeex wants to merge 1 commit into
nodejs:mainfrom
zeexzeex:h2-internal-error-test

Conversation

@zeexzeex

Copy link
Copy Markdown
Contributor

A stream reset before the response headers arrive settles the request promise
with ERR_HTTP2_STREAM_ERROR, carrying the reset code in http2ErrorCode.
That is a different path from a plain stream.close(), which surfaces an
InformationalError:

server sends code http2ErrorCode
stream.close() UND_ERR_INFO
stream.close(INTERNAL_ERROR) ERR_HTTP2_STREAM_ERROR 2
stream.close(CANCEL) UND_ERR_INFO

test/http2-destroy-after-failed-stream.js covers the first row, and the only
existing use of NGHTTP2_INTERNAL_ERROR in the test suite is a session-level
goaway(). The stream-level reset path is currently untested.

Changes

Adds two tests:

  • a request whose stream is reset with NGHTTP2_INTERNAL_ERROR rejects with
    the expected code and http2ErrorCode
  • the failed stream does not prevent other requests on the same connection
    from completing

No production code is changed; this only pins down existing behaviour.

I verified the tests actually catch a regression by removing the
state.abort(err) call in onError (reproducing the behaviour reported in
#2675) and confirming both tests fail, then restoring it.

Refs: #2675

A stream reset before the response headers arrive settles the request
promise with ERR_HTTP2_STREAM_ERROR carrying the reset code. This is a
different path from a plain stream.close(), which surfaces an
InformationalError, so the existing coverage does not exercise it.

Add tests asserting that such a request rejects with the expected code
and that the failed stream does not prevent other requests on the same
connection from completing.

Refs: nodejs#2675
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.46%. Comparing base (fc3450d) to head (ce3aa44).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5725      +/-   ##
==========================================
- Coverage   93.47%   93.46%   -0.01%     
==========================================
  Files         110      110              
  Lines       38908    38908              
==========================================
- Hits        36368    36366       -2     
- Misses       2540     2542       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants