Repository navigation
test(axum-rest-api-server): [#2637] cover the v1 routes, the router layers and the health check - #2687
Conversation
…ter torrust#2667 merged The fixes merged through torrust#2667, so the five rows move from FOLLOW_UP and OPEN to FIXED and RESOLVED and the log records the verification, the replies and the resolution of the threads. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n test plan Records S1 as merged, starts S2 on its branch, updates the EPIC row, and adds the plan for v1/routes.rs: capability gating over private, listed and persistence, always-present read services, and a route registration table. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…wo lines of defense Records the maintainer's decisions: a crate-private test_support helper per ADR rule C, the no-persistence cases kept as the second line of defense linked to the bootstrap validation, and the design question and a stale comment as lessons rows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tion Adds in-process tests of the capability gating over private, listed and persistence (including the no-persistence second line of defense), the always-present read routes, and a registration table of the four contexts. Unit-only lines of v1/routes.rs go from 27 / 45 to 111 / 111; six named mutations are killed. Adds a crate-private test_support helper that builds the REST API container. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Plan for routes.rs, the second file of S2: the authentication layer wiring and the public health check, request id generation and propagation, compression, and the two open questions about the 408 timeout seam and asserting log events. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…stability refactor Records the maintainer's answers: no unit-level log assertions, and a behavior-preserving refactor of routes.rs (R0) so the 408 timeout can be tested; extends the scope with that one exception. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…yers from the routes Moves the layer chain of routes::router into a private with_layers that takes the timeout as a parameter, so a test can reach the 408 handler with a short timeout and its own route. The chain, its order and the default 5 s timeout are unchanged, and the unit and integration tests pass unchanged. Approved by the maintainer on 2026-10-28 as a testability refactor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds in-process tests of the authentication layer wiring with the public health check, request id generation and propagation, compression, and the 408 timeout through the new with_layers seam. Unit-only lines of routes.rs go from 58 / 67 to 174 / 174; eight of ten named mutations are killed and the two survivors are recorded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Plan for the last file of S2: one literal assertion of the status, content type and body of the health check answer, which no test asserted although the lines were already covered. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dler Asserts the status, content type and body of the health check answer as literals; the lines were already covered but nothing asserted the answer. The mutation that reports an error status is killed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…pdate the EPIC tables Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e reader in the router tests Renames the helper that reads a response to into_received so the timeout test no longer binds a variable named like it, and renames the rejection test to cover both a missing and a wrong token. Raised by the independent S2 review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ent review Logs the row 4 approvals, labels the pre-rebase package figures, drops an unasserted content type from the routes plan, narrows the note on log coverage, and records the S2 review report. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… spec and the EPIC Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open issues
What changed in this PR
Adds unit tests and supporting infrastructure to improve coverage of the Axum REST API server’s v1 route composition, router layer wiring, and health check behavior, with a small behavior-preserving refactor to make the timeout path testable.
Changes:
- Added in-file unit tests for
v1route composition (capability gating, persistence/no-persistence behavior, and route registration table). - Refactored
routes::routerto extract a privatewith_layers(..., timeout)and added unit tests for auth wiring, request-id propagation, compression, and408timeout handling. - Added a crate-private
test_supporthelper for constructing the REST API container in unit tests, plus documentation/log updates in the issue/EPIC/audit docs.
| File | Description |
|---|---|
| packages/axum-rest-api-server/src/v1/routes.rs | Adds unit tests for v1 route composition and capability gating. |
| packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs | Adds a unit test asserting the handler’s JSON response shape and headers. |
| packages/axum-rest-api-server/src/test_support.rs | Introduces a unit-test-only helper to build the API container similarly to startup. |
| packages/axum-rest-api-server/src/routes.rs | Extracts with_layers to parameterize timeout and adds unit tests for router layers. |
| packages/axum-rest-api-server/src/lib.rs | Registers the new crate-private test support module under cfg(test). |
| docs/pr-reviews/pr-2638-review/PR-REVIEW.md | Updates audit rows and narrative to reflect follow-up PR merge/resolution. |
| docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/*.md | Adds/updates per-file test plans and index entries for S2. |
| docs/issues/open/2637-1347-add-tests-axum-rest-api-server/* | Updates issue logs, lessons learned, evidence, and reviewer report for S2. |
| docs/issues/open/1347-overhaul-packages-testing/EPIC.md | Updates epic progress and coverage deltas reflecting S2 changes. |
Review effort: Lite
…ithout indexing in the test container helper api_container indexed the first HTTP and UDP tracker with [0], which panics out of bounds with a misleading message when the list is empty. Select it with first() and an explicit expect, and avoid cloning the whole list. Raised as finding F2 of PR torrust#2687. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…the tests read The test helpers read response bodies with usize::MAX as the limit. Add one MAX_BODY_BYTES (1 MiB) to the crate-private test_support module and use it in all five helpers, so an unexpectedly large body fails the test with a clear message instead of being buffered. Raised as finding F1 of PR torrust#2687. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
ACK b2bb60d |
da2ce7
left a comment
da2ce7
left a comment
There was a problem hiding this comment.
Reviewed at b2bb60d3d2890053d2e312669351f5f7c3f8367e (post-merge). Recomputed from the bytes at the merged head, against the base fb278a684; the PR merged on 2026-10-28 at 18:10:07Z as aace1874f.
Label: POST_MERGE_FINDINGS — Review requested at 2026-10-28 17:08:05Z. The PR merged before this round ran; the findings below stand at the merged head for a follow-up, and no ACK follows a merged PR.
Change and tests: inspected all seven changed Rust files before and after. The production refactor preserves the layer chain and order, public health check placement, instrumentation and five-second default; only the private timeout parameter and prefix constant are new. The helper is crate-private and doubly gated for tests. There are 19 v1 composition cases, nine router-layer cases and one direct health-check case. All 11 explicit method/path registrations from the four contexts have requests in the registration table; no explicit v1 endpoint is omitted. That table checks registration only, leaving handler contracts for later slices.
Sensitivity: disabling the private service fails the 200 key case; using private for the whitelist gate fails the listed-only case; removing stats registration fails its table entries; authenticating the public health check fails its 200 case; removing propagation fails the client-id assertion; removing compression fails the gzip case; removing the timeout fails after the outer five-second deadline; changing health status fails the literal JSON body. The default timeout duration and redundant inner request-id layer remain explicitly unprotected. Body reads have a shared 1 MiB limit. No socket is bound; 127.0.0.1:1212 is only logging metadata. Ordinary container/database awaits lack a test-level deadline; only the intentionally pending request is explicitly bounded. The new disk-backed fixture lacks cleanup (F3).
Claims and records: recomputed 17 commits, 18 files, +1124/-36; source changes agree with the limited refactor claim. Ledger rows 2-4 and their plans agree on test counts. Coverage fractions and EPIC deltas are arithmetically consistent; measured totals include test modules, the helper and an independently merged server change. These recorded measurements and historical mutation executions were not rerun. The health-check claim is correctly limited to added protection, not new production-line coverage. Relative Markdown targets and heading fragments resolve. No archive is added. Final metadata stamps precede their carrying commit. The PR #2638 audit transitions to FIXED after PR #2667 merged, making commit subjects appropriate there; its external reply/resolution events are not present in this capture.
The review bot: review 5686737218 contains two inline findings. F1 (4403739901) correctly identifies unbounded body buffering; commit 17107a594 changes all five readers, including the S1 files. F2 (4403739958) correctly identifies the empty-vector indexing panic; 14f4071c2 uses .first() for both lists. Replies 4403849003 and 4403849394 match their source threads; both are resolved in the capture. Resolution subjects exist and audit commit b2bb60d3d follows the replies. The recorded 56-test limit mutation is an execution claim, not independently verified here.
Hygiene: Conventional subjects and applicable issue tags, matching author/committer identities, attribution footer lines without a located prohibition, clean diff whitespace checks, and no closing-pattern match in the description or commits. New vocabulary was compared with the project dictionary; spelling execution remains downstream.
Findings: F3, Minor — retain an owner that removes temporary SQLite files; F4, Minor — bound the new database-backed test awaits.
Checked, no finding: layer-preserving extraction, all explicit route registrations, capability gating, literal health response, timeout-test termination and both bot fixes. This slice extends PR #2667 and tightens its body readers; the PR #2656 rejection remains intact. No builds or runtime tests were run in this lane.
Checks
On the loop's compute hub at the merged head, against the base fb278a684 (receipt server-gates-sojka-476; the PR merged as aace1874f): pre-commit profile gate exit 0 (98 s); linter markdown, linter cspell, linter lychee exit 0; frontmatter-validator --all: 16 errors / 4 warnings at the merged head and 16 / 4 at the base re-measured, the same 20 diagnostics by path and category; the audit validator over docs/pr-reviews/pr-2687-review/PR-REVIEW.md with the review comments and commit list captured at gate time: ok, 2 rows, 2 log entries, 0 failures; cargo test at the merged head: torrust-tracker-axum-rest-api-server 134 passed, 0 failed (exit 0, 74 s; 105 at the base); cargo clippy over torrust-tracker-axum-rest-api-server with all targets and features and -D warnings: exit 0, 0 diagnostics. GitHub checks at this head at posting time: "Publish (Release): completed/skipped; Publish (Development): completed/skipped; Context: completed/success; Package Coverage Regression: completed/success; Package Coverage (torrust-tracker-axum-rest-api-server): completed/success; Docker E2E: completed/skipped; Workspace Packaging: completed/success; Layer Boundary Bans: completed/success; Unit (nightly): completed/success; Unit (stable): completed/success; Shipped Features: completed/success; Documented Clippy Allows: completed/success; Test (Docker) (release): completed/success; Build on windows-latest (nightly): completed/success; Build on windows-latest (stable): completed/success; Build on ubuntu-latest (stable): completed/success; Build on macos-latest (stable): completed/success; Docs Lint: completed/success; Build on macos-latest (nightly): completed/success; Build on ubuntu-latest (nightly): completed/success; Discover Package Coverage: completed/success; Generate Coverage Report: completed/success". At posting, develop is 968099d93.
| /// # Panics | ||
| /// | ||
| /// Panics if the configuration enables no HTTP tracker, no UDP tracker or no HTTP API. | ||
| pub async fn api_container(configuration: &Configuration) -> Arc<TrackerHttpApiCoreContainer> { |
There was a problem hiding this comment.
[Minor][F3] Give the temporary database a cleanup owner
The new callers pass ephemeral() configurations into this helper, which initializes a real SQLite database, but the returned Arc owns no temporary-file guard. packages/test-helpers/src/configuration.rs:85-88 only generates a random path under the temporary directory; it does not create a self-cleaning file. The SQLite driver uses create_if_missing(true), and neither this helper nor the container/driver removes that path. Consequently the four persistence-enabled v1 cases and eight ordinary router cases leave database files behind on every run, multiplying during the documented mutation and coverage loops. Keep a temporary-directory/file guard alive through container use and release the database resources before cleanup, including on assertion failure; the router cases that need no persistence can also avoid creating a database.
| let router = add("/api", Router::new(), &container); | ||
| let request = Request::builder() | ||
| .method(method) | ||
| .uri(uri) |
There was a problem hiding this comment.
[Minor][F4] Bound the database-backed fixture and request awaits
The new call helper awaits real database initialization and oneshot directly. Unlike the separate slow-route test, these cases have no outer deadline, and v1::routes::add deliberately omits the production timeout layer. A stalled database operation or non-returning handler therefore hangs this test instead of producing a bounded failure. The issue's Design and Ownership Review requires an absolute deadline for every awaited operation in tests involving asynchronous I/O. Apply one deadline across container construction, the request and body consumption, and cover the corresponding container-setup path in through_the_router as well. Verify the guard by temporarily replacing the awaited operation with a pending future.
Related to #2637
Slice S2 of the
torrust-tracker-axum-rest-api-serverpackage tests (subissue of EPIC #1347): the v1 route composition, the router layers and the health check handler. Tests, one behavior-preserving refactor and documentation change; no behavior change.Summary
v1/routes.rs: 19 test cases over the capability gating (private, listed, persistence), including the no-persistence states as the second line of defense behind the startup validationvalidate_persistence_requirements, the always-present read routes, and a registration table of the four contexts. Unit-only lines 27 / 45 to 111 / 111; six named mutations killed.routes.rs: 9 test cases over the authentication layer wiring with the public health check, request id generation and propagation, compression, and the408timeout. Unit-only lines 58 / 67 to 169 / 169. Eight of ten named mutations are killed; the two survivors are recorded (a redundant secondSetRequestIdLayer, and the unasserted 5 s default).v1/context/health_check/handlers.rs: one test of the status, content type and body. The file was already fully covered, so this adds protection, not lines.src/test_support.rshelper (a#[cfg(test)]module with an inner#![cfg(test)], ADR20261021190318rule C) builds the REST API container for the router tests.refactor(axum-rest-api-server): [#2637] separate the router layers from the routes): the layer chain ofroutes::routermoved into a privatewith_layersthat takes the timeout, so a test can reach the408handler. A pure move: same chain, same order, same 5 s default, same#[instrument]; the 58 integration tests and the existing unit tests passed unchanged.lessons.md(the design question about an unrepresentable state, thetest_file convention for a later ADR clarification, and two observations), and the independent Task Reviewer report (REVIEW PASSED, with its findings fixed).develop, the five threads are replied to and resolved, and the audit rows areFIXEDandRESOLVED.Coverage (stable Rust 1.99.1,
cargo-llvm-cov 0.6.16, filtered topackages/axum-rest-api-server/src/)The totals count the new in-file test modules and the helper.
developalso changedserver.rswhile S2 ran (the configured drain timeout and its test), which is part of the rise and is described in the evidence.Files
packages/axum-rest-api-server/(src/routes.rs,src/v1/routes.rs,src/v1/context/health_check/handlers.rs,src/lib.rs,src/test_support.rs),docs/issues/open/2637-1347-add-tests-axum-rest-api-server/,docs/issues/open/1347-overhaul-packages-testing/EPIC.md,docs/pr-reviews/pr-2638-review/PR-REVIEW.md.Validation
All on the rebased branch:
./contrib/dev-tools/git/hooks/pre-commit.shpassed for every commit and./contrib/dev-tools/git/hooks/pre-push.shpassed (nightly format and check, nightly docs, packaging check, full stable test suite).cargo test -p torrust-tracker-axum-rest-api-serverpassed (76 unit and 58 integration tests),cargo +nightly fmt --all -- --check,cargo clippy -p torrust-tracker-axum-rest-api-server --all-targetsandcargo machete --with-metadatapassed. The history was rebased ontodevelopbefore the push; no commit was squashed.