From 15984bb8c746a2b0e8a169f67ccd3e8ed0fda83d Mon Sep 17 00:00:00 2001 From: Ashish Sharma Date: Thu, 27 Aug 2026 16:49:21 +0800 Subject: [PATCH] test(esp_http_client): final review fixes for characterization suite Runner now runs unfiltered; #error guard couples BASIC_AUTH to custom transport configs; CMake include and REQUIRES cleanup; doc corrections and tightened asserts from the final review. --- .../test_apps/main/CMakeLists.txt | 3 +- .../test_apps/main/test_http_client.c | 2 +- .../test_apps/main/test_http_client_auth.c | 4 +++ .../test_apps/main/test_http_client_basic.c | 19 +++++++++++++ .../main/test_http_client_lifecycle.c | 5 ++++ .../main/test_http_client_streaming.c | 28 +++++++++++++++++++ .../test_http_client_mock_transport.h | 8 ++++-- .../test_apps/pytest_stage0_qemu.py | 5 +--- .../test_apps/sdkconfig.ci.default | 4 --- .../test_apps/sdkconfig.defaults | 3 ++ 10 files changed, 68 insertions(+), 13 deletions(-) create mode 100644 components/esp_http_client/test_apps/sdkconfig.defaults diff --git a/components/esp_http_client/test_apps/main/CMakeLists.txt b/components/esp_http_client/test_apps/main/CMakeLists.txt index da5ce1d4f4e..4a8e9bddfda 100644 --- a/components/esp_http_client/test_apps/main/CMakeLists.txt +++ b/components/esp_http_client/test_apps/main/CMakeLists.txt @@ -3,7 +3,6 @@ set(mock_transport_dir ${CMAKE_CURRENT_SOURCE_DIR}/../mock_transport) idf_component_register(SRC_DIRS "." ${mock_transport_dir} PRIV_INCLUDE_DIRS "." "../../lib/include" + ${mock_transport_dir} PRIV_REQUIRES esp_http_client tcp_transport test_utils unity - INCLUDE_DIRS ${mock_transport_dir} - REQUIRES tcp_transport WHOLE_ARCHIVE) diff --git a/components/esp_http_client/test_apps/main/test_http_client.c b/components/esp_http_client/test_apps/main/test_http_client.c index d5cf839633a..ece6185d50a 100644 --- a/components/esp_http_client/test_apps/main/test_http_client.c +++ b/components/esp_http_client/test_apps/main/test_http_client.c @@ -465,7 +465,7 @@ static const char *mock_http_response_ok = "\r\n" "{\"status\":\"ok\"}"; -esp_err_t _http_event_handler(esp_http_client_event_t *evt) +static esp_err_t _http_event_handler(esp_http_client_event_t *evt) { switch (evt->event_id) { case HTTP_EVENT_ON_CONNECTED: diff --git a/components/esp_http_client/test_apps/main/test_http_client_auth.c b/components/esp_http_client/test_apps/main/test_http_client_auth.c index 3d042506a91..09deb5abb31 100644 --- a/components/esp_http_client/test_apps/main/test_http_client_auth.c +++ b/components/esp_http_client/test_apps/main/test_http_client_auth.c @@ -52,6 +52,10 @@ */ #if CONFIG_ESP_HTTP_CLIENT_ENABLE_CUSTOM_TRANSPORT +#if !CONFIG_ESP_HTTP_CLIENT_ENABLE_BASIC_AUTH +#error "These cases require CONFIG_ESP_HTTP_CLIENT_ENABLE_BASIC_AUTH=y; keep every sdkconfig.ci.* with CUSTOM_TRANSPORT in sync" +#endif + static const char *resp_401 = "HTTP/1.1 401 Unauthorized\r\n" "WWW-Authenticate: Basic realm=\"Test\"\r\n" diff --git a/components/esp_http_client/test_apps/main/test_http_client_basic.c b/components/esp_http_client/test_apps/main/test_http_client_basic.c index b3e8dc8df03..ceb0618683d 100644 --- a/components/esp_http_client/test_apps/main/test_http_client_basic.c +++ b/components/esp_http_client/test_apps/main/test_http_client_basic.c @@ -496,6 +496,25 @@ TEST_CASE("Client handles 401 Unauthorized error", "[esp_http_client][basic][p0] TEST_ASSERT_EQUAL(ESP_OK, mock_http_transport_get_stats(mock_transport, &stats)); TEST_ASSERT_EQUAL(2, stats.write_calls); + /* characterization: master behavior, see refactor spec + * The queued second 401 triggers a queue-advance on the retry's write + * (previous response fully read, a queued response is still pending - + * see test_http_client_mock_transport.h), which resets the capture + * buffer just before the retry is written. So this last-request capture + * holds only the credential-less retry, not the original request. + * Confirms no Authorization header is attached: no auth_type, + * username/password, or URL-embedded credentials were ever configured + * on this client, so esp_http_client_prepare()'s credential check + * (~L800-803) has nothing to attach even though add_auth() unconditionally + * schedules the retry. */ + char req[2048]; + TEST_ASSERT_EQUAL(ESP_OK, mock_http_transport_get_last_request(mock_transport, req, sizeof(req), NULL)); + /* Non-vacuity check: confirms the capture actually holds the retried + * request line (not empty, not the original request left over from a + * missed reset) before trusting the Authorization-absence assert below. */ + TEST_ASSERT_NOT_NULL(strstr(req, "GET /api/protected")); + TEST_ASSERT_NULL(strstr(req, "Authorization")); + ESP_LOGI(TAG, "OK: 401 error handled, credential-less retry capped and reported"); esp_http_client_cleanup(client); diff --git a/components/esp_http_client/test_apps/main/test_http_client_lifecycle.c b/components/esp_http_client/test_apps/main/test_http_client_lifecycle.c index 6d1da3ee559..eea74bdb886 100644 --- a/components/esp_http_client/test_apps/main/test_http_client_lifecycle.c +++ b/components/esp_http_client/test_apps/main/test_http_client_lifecycle.c @@ -81,6 +81,11 @@ TEST_CASE("Connection: close forces reconnect on next perform", "[esp_http_clien mc.response_data = resp_close; esp_transport_handle_t mock = mock_http_transport_create(&mc); TEST_ASSERT_NOT_NULL(mock); + /* characterization: this queued response is never popped - the forced + * reconnect calls mock_close() then mock_connect(), both of which reset + * read_offset to 0, so the second perform() re-serves the identical + * initial buffer instead of advancing the queue; kept here for + * intent-documentation. */ mock_http_transport_queue_response(mock, resp_close, 0); esp_http_client_config_t cfg = { diff --git a/components/esp_http_client/test_apps/main/test_http_client_streaming.c b/components/esp_http_client/test_apps/main/test_http_client_streaming.c index eb61f244e98..ec2082dfe2a 100644 --- a/components/esp_http_client/test_apps/main/test_http_client_streaming.c +++ b/components/esp_http_client/test_apps/main/test_http_client_streaming.c @@ -55,12 +55,40 @@ TEST_CASE("open/write/fetch_headers/read sequence works", "[esp_http_client][str const char *body = "abcde"; TEST_ASSERT_EQUAL(ESP_OK, esp_http_client_open(client, 5)); + // characterization: master behavior, see refactor spec + // esp_http_client_open() (esp_http_client.c ~L1921) sets + // state = HTTP_STATE_REQ_COMPLETE_HEADER right after writing the + // request line and headers over the transport. + TEST_ASSERT_EQUAL(HTTP_STATE_REQ_COMPLETE_HEADER, esp_http_client_get_state(client)); TEST_ASSERT_EQUAL(5, esp_http_client_write(client, body, 5)); + // characterization: master behavior, see refactor spec + // The public esp_http_client_write() (~L1978) never touches + // client->state - it only requires state >= REQ_COMPLETE_HEADER and + // writes bytes directly over the transport, so the state observed + // here is unchanged from the open() call above. + TEST_ASSERT_EQUAL(HTTP_STATE_REQ_COMPLETE_HEADER, esp_http_client_get_state(client)); TEST_ASSERT_EQUAL(5, esp_http_client_fetch_headers(client)); + // characterization: master behavior, see refactor spec + // esp_http_client_fetch_headers() (~L1667-1697) unconditionally sets + // state = HTTP_STATE_REQ_COMPLETE_DATA on entry, then reads/parses + // until the response header-parse loop exits, then unconditionally + // sets state = HTTP_STATE_RES_ON_DATA_START before returning - it + // never stops at RES_COMPLETE_HEADER and does not depend on whether + // any body bytes were actually read yet. + TEST_ASSERT_EQUAL(HTTP_STATE_RES_ON_DATA_START, esp_http_client_get_state(client)); TEST_ASSERT_EQUAL(200, esp_http_client_get_status_code(client)); char buf[16] = {0}; int rd = esp_http_client_read(client, buf, sizeof(buf)); + // characterization: master behavior, see refactor spec + // esp_http_client_read() (~L1435) never assigns client->state at all. + // Also, for this canned response the single mock transport read done + // inside fetch_headers() above already delivered the whole 43-byte + // buffer (headers + 5-byte body) to the parser in one + // http_parser_execute() call, so this read() serves the body from the + // already-cached response buffer without issuing a second transport + // read - the state observed here is unchanged from fetch_headers(). + TEST_ASSERT_EQUAL(HTTP_STATE_RES_ON_DATA_START, esp_http_client_get_state(client)); TEST_ASSERT_EQUAL(5, rd); TEST_ASSERT_EQUAL_STRING("hello", buf); TEST_ASSERT_TRUE(esp_http_client_is_complete_data_received(client)); diff --git a/components/esp_http_client/test_apps/mock_transport/test_http_client_mock_transport.h b/components/esp_http_client/test_apps/mock_transport/test_http_client_mock_transport.h index 1a202b7a08b..ca548b6ecf5 100644 --- a/components/esp_http_client/test_apps/mock_transport/test_http_client_mock_transport.h +++ b/components/esp_http_client/test_apps/mock_transport/test_http_client_mock_transport.h @@ -202,8 +202,12 @@ esp_err_t mock_http_transport_queue_response(esp_transport_handle_t t, * @brief Retrieve the bytes captured from the most recent request * * mock_write() appends every written byte (capped at 2048 bytes) into an - * internal capture buffer, reset at each request boundary. This lets tests - * assert on the serialized request content (headers, body). + * internal capture buffer. The buffer is reset only on a queue-advance: a + * write arriving after the current response has been fully read AND a + * queued response is still pending (see mock_http_transport_queue_response()). + * With the queue empty or exhausted, no reset happens and the capture + * concatenates bytes across requests. This lets tests assert on the + * serialized request content (headers, body). * * @param[in] t Mock transport handle * @param[out] buf Buffer to receive the captured request bytes, NUL-terminated diff --git a/components/esp_http_client/test_apps/pytest_stage0_qemu.py b/components/esp_http_client/test_apps/pytest_stage0_qemu.py index 22b058de9bd..f7d7b3d9434 100644 --- a/components/esp_http_client/test_apps/pytest_stage0_qemu.py +++ b/components/esp_http_client/test_apps/pytest_stage0_qemu.py @@ -8,7 +8,4 @@ from pytest_embedded_idf.utils import idf_parametrize @pytest.mark.qemu @idf_parametrize('target', ['esp32c3'], indirect=['target']) def test_http_client_mock(dut: Dut) -> None: - dut.run_all_single_board_cases( - group=['basic', 'async', 'lifecycle', 'chunked', 'redirect', 'auth', 'streaming', 'error_recovery'], - timeout=600, - ) + dut.run_all_single_board_cases(timeout=600) diff --git a/components/esp_http_client/test_apps/sdkconfig.ci.default b/components/esp_http_client/test_apps/sdkconfig.ci.default index 9248be8bca7..0e42092563e 100644 --- a/components/esp_http_client/test_apps/sdkconfig.ci.default +++ b/components/esp_http_client/test_apps/sdkconfig.ci.default @@ -17,7 +17,3 @@ CONFIG_ESP_HTTP_CLIENT_ENABLE_CUSTOM_TRANSPORT=y # (unencrypted credentials without TLS); this is a test-only config, not a # client-code change - the mock transport never touches the network. CONFIG_ESP_HTTP_CLIENT_ENABLE_BASIC_AUTH=y - -# Some of the test apps allocate 2048 size buffers on the stack. -# Increase the default stack size to accommodate for that -CONFIG_ESP_MAIN_TASK_STACK_SIZE=8192 diff --git a/components/esp_http_client/test_apps/sdkconfig.defaults b/components/esp_http_client/test_apps/sdkconfig.defaults new file mode 100644 index 00000000000..e69fdbe645e --- /dev/null +++ b/components/esp_http_client/test_apps/sdkconfig.defaults @@ -0,0 +1,3 @@ +# Some of the test apps allocate 2048 size buffers on the stack. +# Increase the default stack size to accommodate for that +CONFIG_ESP_MAIN_TASK_STACK_SIZE=8192