test(http2): drive connected-state paths via mock_h2_server_peer (Part of #1062)#1076
Merged
Merged
Conversation
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
5 tasks
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.
Contributor
Coverage Report
Coverage DetailsFull HTML report is available as a build artifact. |
This was referenced Apr 28, 2026
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
Adds 12 new
TEST_Fcases under the existingHttp2ClientHermeticTransportTestfixture intests/unit/http2_client_branch_test.cpp(+367 LOC, no new test files) that compose the Phase 2Amock_h2_server_peershipped in PR #1075 to bringhttp2_clientinto a fully SETTINGS-exchanged state, then exercise public methods that previously early-returned viais_connected()in the existing disconnected-state suites.Newly reached paths in
src/protocols/http2/http2_client.cppconnect()second-call short-circuit through thealready_existsbranch (lines 81-88)send_request()/get()/post()create_stream+build_headers+encoder_.encode+send_frameon the connected path, terminating in thefuture.wait_for(timeout_)branch (lines 723-844, especially 817-826)start_stream()/write_stream()/cancel_stream()/close_stream_writer()connected paths plus theirnot_founderror branches with an unallocated stream id (lines 306-462)close_stream_writer()idempotent early-return on ahalf_closed_localstream (lines 412-415)set_settings()post-handshake call site updating HPACK encoder / decoder table sizes (lines 299-304)disconnect()GOAWAY emit +stop_iopath followed by the second-call early-return on a torn-down handshaken client (lines 210-237)Why
Part of #1062andPart of #953(epic: 40% → 80% coverage).http2_client.cppline 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 driveis_connected() == truein 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 expandhttp2_client.cppcoverage.The acceptance criteria of
>= 80% line / >= 70% branchis not reached by this PR alone — it cannot be, until Phase 2A.2 of #1074 landsmock_h2_server_peerHEADERS+DATA reply support. Without server replies, allhandle_headers_frame/handle_data_frame/handle_rst_stream_frame/handle_goaway_frame/handle_window_update_frame/handle_ping_framepaths remain unreachable from a hermetic test, and the happy-path completion ofsend_request()(status code extraction,promise.set_value(...)) cannot be driven. This PR therefore usesPart of #1062, notCloses.Where
tests/unit/http2_client_branch_test.cppmake_connected_client(peer, client_id, request_timeout)plus 12 newTEST_Fcases appended after the existing Phase 2A demoNo changes under
src/, no new test files, no CMake change (the file is already inadd_network_test(http2_client_branch_test ...)andnetwork::test_supportis already linked from PR #1075).How
Each new
TEST_F:mock_h2_server_peer peer(io())on the fixture's sharedio_context.make_connected_client(peer, client_id)which spawns a worker thread that runsclient->connect("127.0.0.1", peer.port())synchronously.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'sis_connected_is therefore true.Result<T>outcome.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 msset_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.ymlworkflow is the authoritative verification surface (PR #1071 / #1075 precedent). Expected outcome:http2_client.cppline coverage strictly increases above the 18.8% baseline.Part of, notCloses); the issue's>= 80% line / >= 70% branchacceptance criteria becomes reachable only after Phase 2A.2 lands.Part of #1062
Part of #953