test(http2): add frame_injector-driven error coverage round 2 (#1106)#1108
Merged
Merged
Conversation
Adds 6 new TEST_F cases to tests/unit/http2_client_branch_test.cpp that compose mock_h2_server_peer (Phase 2A/2A.2/2E substrate) with the Phase 2E frame_injector (PR #1105) to drive previously-unreachable error branches in src/protocols/http2/http2_client.cpp. The Round 1 sub-issues of #953 (#991, #1062) raised happy-path coverage on http2_client.cpp but left the file at 18.8% line / 9.9% branch on the 2026-05-06 develop measurement (run 25430202846). Round 1 closed before the frame_injector substrate existed, so error-path branches that require deterministic server-side frame faults could not be exercised. This change opts each new TEST_F into one of five injection_mode values: - DropFirstServerSettingsLeavesClientUnconnected injection_mode::drop on the first server-originated frame; client handshake never completes, exercising the connect-timeout branch that was previously only reachable via DISABLED integration tests. - TruncatedServerSettingsHeaderTimesOutConnect injection_mode::truncate at 4 bytes; client receives an incomplete 9-byte frame header and waits indefinitely, exercising the short-read / partial-header branch in the read loop. - MalformedServerSettingsTypeByteTriggersConnectTimeout injection_mode::malform XOR=0x0F on the type-byte (offset 3); turns SETTINGS into an unknown frame type (0x0B), exercising the process_frame default / unknown-type dispatch branch. - MalformedServerSettingsStreamIdNonZeroIsProtocolError injection_mode::malform XOR=0x01 on the last stream-id byte (offset 8); turns SETTINGS frame stream_id from 0 to 1, which is a PROTOCOL_ERROR per RFC 7540 §6.5, exercising the connection-error branch in handle_settings_frame. - SlowWriteServerSettingsStillCompletesHandshake injection_mode::slow_write step=500us; positive test that the accumulating-buffer / partial-read branch in the read loop still completes the handshake when bytes arrive byte-by-byte. - EchoOneTruncateAtNineDropsResponsePayloadFailingGet injection_mode::truncate at 9 bytes combined with reply_mode::echo_one; SETTINGS exchange completes (empty SETTINGS frames are exactly 9 bytes so truncate_at=9 is a no-op for them) but response HEADERS and DATA frames lose their payloads, exercising the request-error / timeout branch on the connected path. No src/ changes. Default injection_spec{} (== injection_mode::none) on mock_h2_server_peer preserves Phase 2A/2A.2 behavior for all existing TEST_F call sites that do not supply a spec. Local verification: build clean, 4/6 new tests pass on macOS Apple Silicon with the same pre-existing hermetic-handshake instability that was observed and documented in PR #1105. The 2 tests with positive is_connected() assertions fail locally for the same environmental reason that fails the unmodified develop-tip baseline test ConnectCompletesSettingsExchangeWithMockPeer on this machine. CI on Linux + GitHub macOS runners is the authoritative verification surface. Part of #1106 Part of #953
Contributor
Coverage Report
Coverage DetailsFull HTML report is available as a build artifact. |
kcenon
added a commit
that referenced
this pull request
May 6, 2026
#1109) * test(support): add post-handshake server-frame hook to mock_h2_server_peer Adds an optional post_handshake_frames constructor argument that lets callers supply pre-serialized HTTP/2 frame byte buffers for the mock peer to write immediately after the SETTINGS-ACK and before the per-mode request/response handling. Each entry is written verbatim, bypassing the frame_injector, so tests can construct precisely-formed PING, GOAWAY, WINDOW_UPDATE, RST_STREAM or arbitrary unknown-type frames whose wire format frame::parse will accept and route through the client's process_frame dispatcher. This unlocks dispatcher-branch coverage paths in http2_client.cpp that the request-error path (echo_one + injector) cannot reach because those branches require a server-originated frame on the connected state, not a corrupted response to a client-initiated request. Default empty value preserves existing Phase 2A / 2A.2 / 2E.R2 behavior for all current call sites. * test(http2): guard coverage-flaky tests, add dispatcher branch tests Phase 2E.R3 follow-up to PR #1108. The Round 2 work landed 6 new TEST_F cases on the http2_client.cpp branch test, but the post-merge coverage workflow showed line/branch coverage on http2_client.cpp unchanged at 18.8% / 9.9% (target 60% / 40%). Root cause: 2 positive-path TEST_F (slow_write handshake, echo_one + truncate-at-9 response) fail under coverage instrumentation because lcov/gcov slow the SETTINGS handshake past the 2-3 second wait budget. The other 4 TEST_F pass but only re-cover early-connect lines that existing happy-path tests already hit, so they contribute zero net coverage. handle_ping_frame, handle_goaway_frame, handle_window_update_frame, handle_rst_stream_frame, and process_frame default branch remain unreachable. Two changes: 1. Guard the two coverage-flaky tests with #ifndef NETWORK_COVERAGE_BUILD. tests/CMakeLists.txt defines NETWORK_COVERAGE_BUILD on the branch test target only when ENABLE_COVERAGE is on, so Debug/Release matrix builds keep the slow_write partial-read accumulator and truncate-on- connected request-error branches under test, while the coverage workflow no longer carries a measured-failing-test signal. 2. Add 7 dispatcher TEST_F cases that use the new mock_h2_server_peer post_handshake_frames hook to write server-originated frames after the SETTINGS handshake: - ServerPingFrameDrivesHandlePingAndKeepsConnectionAlive - ServerPingAckFrameIsAbsorbedSilently - ServerGoawayFrameFlipsConnectionStateToDisconnected - ServerWindowUpdateOnConnectionStreamExpandsWindow - ServerWindowUpdateOnUnknownStreamIsSilentlyIgnored - ServerRstStreamOnUnknownStreamIsSilentlyIgnored - ServerUnknownFrameTypeIsHandledWithoutCrashing Each frame is serialized via the production frame class (ping_frame, goaway_frame, window_update_frame, rst_stream_frame) so frame::parse accepts the wire format and routes the frame to the intended handler. The unknown-type case constructs raw bytes with type=0xFF to drive either process_frame's default branch or the run_io read-error path; both outcomes are RFC-compliant. Local Debug build: existing pre-merge hermetic instability on macOS Apple Silicon affects the new tests the same way it affects the unmodified ConnectCompletesSettingsExchangeWithMockPeer baseline (documented in PR #1108 body). CI Linux + GitHub macOS runners are the authoritative verification surface, as before. Closes #1106
This was referenced May 7, 2026
kcenon
added a commit
that referenced
this pull request
May 8, 2026
…ixture (#1113) Scale every hermetic-fixture wait_for budget by NETWORK_COVERAGE_TIMEOUT_MULTIPLIER so Http2ClientHermeticTransportTest dispatcher cases can complete the SETTINGS handshake under -fprofile-arcs -ftest-coverage instrumentation. PR #1111 diagnosis confirmed all 7 Round-3 dispatcher TEST_F (and ~30 other fixture-based tests) failed at wait_for(settings_exchanged(), 3s): 5.1-5.4s observed under coverage vs 100-300ms in Debug/Release, producing 0pp delta on src/protocols/http2/http2_client.cpp for three rounds (PR #1108, PR #1109). Changes: - tests/support/hermetic_transport_fixture.h: define NETWORK_COVERAGE_TIMEOUT_MULTIPLIER (default 1), apply to wait_for default timeout argument. - tests/support/hermetic_transport_fixture.cpp: scale make_loopback_tcp_pair accept-handler deadline by the multiplier. - tests/support/CMakeLists.txt: define NETWORK_COVERAGE_TIMEOUT_MULTIPLIER=5 on network_test_support PUBLIC when ENABLE_COVERAGE is ON, so all consuming test executables inherit it at the point of header inclusion. No src/ changes. No new TEST_F. Multiplier defaults to 1 outside coverage builds; Debug/Release matrix observable behaviour is unchanged. Closes #1112
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Round 2 of
http2_client.cpptest-coverage expansion. Adds 6 new TEST_Fcases to
tests/unit/http2_client_branch_test.cppthat composemock_h2_server_peerwith the Phase 2Eframe_injector(PR #1105) todrive previously-unreachable error branches in
src/protocols/http2/http2_client.cpp.Change Type
src/changes)Affected Components
tests/unit/http2_client_branch_test.cppWhy
http2_client.cppwas at 18.8% line / 9.9% branch on the 2026-05-06develop measurement (run 25430202846),
the single largest contributor to the gap between develop's 69.1% line
coverage and the 80% target tracked by epic #953. The Round 1 sub-issues
(#991, #1062) raised happy-path coverage but closed before the Phase 2E
substrate existed — so error-path branches that require deterministic
server-side frame faults remained unreachable.
PR #1105 (Phase 2E of #1074) shipped
frame_injectorand routed everymock_h2_server_peerserver-originated write through it. This PR is thefirst consumer of that substrate against
http2_client.cpp. The defaultinjection_spec{}producesinjection_mode::none, so the 30+ existingTEST_F call sites that do not supply a spec are unchanged.
Who
grpc/client.cpp(test(grpc): expand client.cpp error-branch coverage using Phase 2 substrate (round 2) #1107)follow the same pattern this PR establishes.
When
frame_injector+mock_h2_server_peer,PRs infra(test): add mock_h2_server_peer for HTTP/2 loopback (Phase 2A of #1074) #1075, infra(test): add mock_grpc_server_peer for gRPC unary loopback (Phase 2B of #1074) #1102, test(support): add frame_injector for protocol-aware fault injection (Phase 2E of #1074) #1105 — all merged).
(test(grpc): expand client.cpp error-branch coverage using Phase 2 substrate (round 2) #1107 for gRPC).
Where
src/is not touched. No public-API change. Nobuild-config change.
Http2ClientHermeticTransportTestfixture under a dedicated sectionheader marked
Phase 2E.R2: frame_injector-driven error coverage.How
Per-test design
DropFirstServerSettingsLeavesClientUnconnecteddropconnect()TruncatedServerSettingsHeaderTimesOutConnecttruncate(4 bytes)MalformedServerSettingsTypeByteTriggersConnectTimeoutmalform(offset 3, XOR 0x0F)process_frameunknown-type dispatchMalformedServerSettingsStreamIdNonZeroIsProtocolErrormalform(offset 8, XOR 0x01)handle_settings_frameconnection-level PROTOCOL_ERRORSlowWriteServerSettingsStillCompletesHandshakeslow_write(500 us step)EchoOneTruncateAtNineDropsResponsePayloadFailingGettruncate(9 bytes) +reply_mode::echo_oneWhy these specific specs
The
frame_injectorapplies one global spec to every server-originatedwrite a peer emits. There is no per-write granularity. Tests therefore
choose specs such that:
Faults that must land on the first frame (drop / truncate / malform
variants targeting the empty 9-byte SETTINGS frame) cause
is_connected()to never flip true within the wait budget. The peer's worker is also
blocked at the read-client-SETTINGS step because the client does not
send its SETTINGS until it has parsed valid server SETTINGS.
truncate_at = 9is a no-op for empty-payload SETTINGS frames(which are exactly 9 bytes total) but truncates response HEADERS and
DATA frames in
reply_mode::echo_one(which are 10 + 11 bytesrespectively, header + payload). This is how the test reaches a
connected state and still observes a request-level error.
slow_writeis a positive test verifying that pacing bytes500 us apart still completes the handshake; this drives the
accumulating-buffer branch that single-shot writes do not exercise.
Local verification
network_test_supportlibrarynetwork_http2_client_branch_testtargetTest execution on macOS Apple Silicon (this machine): 4/6 new tests pass.
The 2 tests with positive
is_connected()assertions(
SlowWriteServerSettingsStillCompletesHandshake,EchoOneTruncateAtNineDropsResponsePayloadFailingGet) fail with the samesymptom as the unmodified
ConnectCompletesSettingsExchangeWithMockPeertest from develop tip —
peer.settings_exchanged()does not flip truewithin the wait budget. This is the same pre-existing local-environment
hermetic instability documented during the PR #1105 review and is not
introduced by this PR (verified via A/B run of the unmodified baseline
test, which also fails on this machine).
CI on Linux + GitHub macOS runners is the authoritative verification
surface for these positive-path assertions.
Test plan for reviewers
Acceptance follow-up (#1106)
The acceptance criteria of #1106 include a coverage-uplift target
(line >= 60%, branch >= 40% on
http2_client.cpp). Coverage is measuredpost-merge by the Code Coverage workflow on develop. This PR's CI will
verify the test correctness; the coverage delta will appear on the
next develop coverage run after merge.
Breaking changes
None.
Rollback plan
Revert this PR. Only
tests/unit/http2_client_branch_test.cppisaffected; existing tests are unchanged.
Related
Checklist
tests/unit/http2_client_branch_test.cppstylesrc/changes