Skip to content

test(http2): drive connected-state paths via mock_h2_server_peer (Part of #1062)#1076

Merged
kcenon merged 2 commits into
developfrom
test/issue-1062-http2-client-coverage-phase2a
Apr 28, 2026
Merged

test(http2): drive connected-state paths via mock_h2_server_peer (Part of #1062)#1076
kcenon merged 2 commits into
developfrom
test/issue-1062-http2-client-coverage-phase2a

Conversation

@kcenon

@kcenon kcenon commented Apr 28, 2026

Copy link
Copy Markdown
Owner

What

Adds 12 new TEST_F cases under the existing Http2ClientHermeticTransportTest fixture in tests/unit/http2_client_branch_test.cpp (+367 LOC, no new test files) that compose the Phase 2A mock_h2_server_peer shipped in PR #1075 to bring http2_client into a fully SETTINGS-exchanged state, then exercise public methods that previously early-returned via is_connected() in the existing disconnected-state suites.

Newly reached paths in src/protocols/http2/http2_client.cpp

  • connect() second-call short-circuit through the already_exists branch (lines 81-88)
  • send_request() / get() / post() create_stream + build_headers + encoder_.encode + send_frame on the connected path, terminating in the future.wait_for(timeout_) branch (lines 723-844, especially 817-826)
  • start_stream() / write_stream() / cancel_stream() / close_stream_writer() connected paths plus their not_found error branches with an unallocated stream id (lines 306-462)
  • close_stream_writer() idempotent early-return on a half_closed_local stream (lines 412-415)
  • set_settings() post-handshake call site updating HPACK encoder / decoder table sizes (lines 299-304)
  • disconnect() GOAWAY emit + stop_io path followed by the second-call early-return on a torn-down handshaken client (lines 210-237)

Why

Part of #1062 and Part of #953 (epic: 40% → 80% coverage).

http2_client.cpp line coverage stood at 18.8% line / 9.9% branch as of the 2026-04-26 lcov run (workflow 24947193873). PR #1068 added 30 disconnected-state TEST_F cases but the remaining gap was concentrated in the post-handshake code that needed an in-process peer to drive is_connected() == true in CI. PR #1075 shipped that peer (mock_h2_server_peer — connection preface read, server SETTINGS send, client SETTINGS read, SETTINGS-ACK send) and added one demo TEST_F. This PR is the first follow-up that uses the peer to actually expand http2_client.cpp coverage.

The acceptance criteria of >= 80% line / >= 70% branch is not reached by this PR alone — it cannot be, until Phase 2A.2 of #1074 lands mock_h2_server_peer HEADERS+DATA reply support. Without server replies, all handle_headers_frame / handle_data_frame / handle_rst_stream_frame / handle_goaway_frame / handle_window_update_frame / handle_ping_frame paths remain unreachable from a hermetic test, and the happy-path completion of send_request() (status code extraction, promise.set_value(...)) cannot be driven. This PR therefore uses Part of #1062, not Closes.

Where

Path Change
tests/unit/http2_client_branch_test.cpp +367 LOC: anonymous-namespace helper make_connected_client(peer, client_id, request_timeout) plus 12 new TEST_F cases appended after the existing Phase 2A demo

No changes under src/, no new test files, no CMake change (the file is already in add_network_test(http2_client_branch_test ...) and network::test_support is already linked from PR #1075).

How

Each new TEST_F:

  1. Constructs a mock_h2_server_peer peer(io()) on the fixture's shared io_context.
  2. Calls make_connected_client(peer, client_id) which spawns a worker thread that runs client->connect("127.0.0.1", peer.port()) synchronously.
  3. Waits on wait_for([&]() { return peer.settings_exchanged(); }, std::chrono::seconds(3)) — by which time the peer has read the preface, sent server SETTINGS, read client SETTINGS, and sent SETTINGS-ACK; client's is_connected_ is therefore true.
  4. Drives the targeted post-connect public method, asserting the expected Result<T> outcome.
  5. Calls disconnect() which emits GOAWAY (drained by peer) and tears down state, then joins the connector thread.

Tests reaching the send_request() timeout branch use a 150 ms set_timeout(...) to bound execution time. Other tests use the default 2 s timeout for the connect path; their post-connect calls return immediately since they don't wait on a future.

Coverage Scope (Honest Assessment)

Local C++ toolchain (cmake/g++/ninja) is not available in this dev environment; the coverage.yml workflow is the authoritative verification surface (PR #1071 / #1075 precedent). Expected outcome:

Part of #1062
Part of #953

Adds 12 new TEST_F cases under Http2ClientHermeticTransportTest in
tests/unit/http2_client_branch_test.cpp that compose the Phase 2A
mock_h2_server_peer (PR #1075) to bring http2_client into a fully
SETTINGS-exchanged state, then exercise public methods that early-
returned via is_connected() in the existing disconnected-state suite.

Newly reached paths in src/protocols/http2/http2_client.cpp:
- connect() second-call short-circuit through already_exists branch
- send_request() / get() / post() create_stream + build_headers +
  encoder_.encode + send_frame on the connected path, terminating in
  the future timeout branch (peer sends no HEADERS+DATA in 2A)
- start_stream() / write_stream() / cancel_stream() /
  close_stream_writer() connected paths plus their not_found error
  branches with an unallocated stream id
- close_stream_writer() idempotent early-return on a half_closed_local
  stream
- set_settings() post-handshake call site updating encoder / decoder
  table sizes
- disconnect() GOAWAY emit + stop_io path followed by the second-call
  early-return on a torn-down handshaken client

HEADERS+DATA reply paths remain unreachable until Phase 2A.2 of #1074
lands; this PR therefore uses Part of #1062, not Closes.

Part of #1062
Part of #953
The four lambdas added in the prior commit used the bare identifier
http_header in their parameter list, which macOS clang flags as
'use of undeclared identifier' because the namespace alias
'namespace http2 = kcenon::network::protocols::http2;' inside the
test file does not implicitly bring symbols of that namespace into
unqualified lookup at the function signature site.

Switch all four occurrences to http2::http_header to match the
existing qualified usage at line 60 of the file.
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

Metric Value
Line Coverage 66.9%
Branch Coverage 33.2%
Target 80% lines / 70% branches
Coverage Details

Full HTML report is available as a build artifact.

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.

1 participant