From 0cd07963697aa3e5e0cfc614e978da4b8d9304b4 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 27 Oct 2026 14:57:05 +0000 Subject: [PATCH 01/17] docs(pr-reviews): close the PR #2638 late findings F5 to F9 after #2667 merged The fixes merged through #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> --- docs/pr-reviews/pr-2638-review/PR-REVIEW.md | 23 +++++++++++---------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/pr-reviews/pr-2638-review/PR-REVIEW.md b/docs/pr-reviews/pr-2638-review/PR-REVIEW.md index 53ddc66f4..9478725a2 100644 --- a/docs/pr-reviews/pr-2638-review/PR-REVIEW.md +++ b/docs/pr-reviews/pr-2638-review/PR-REVIEW.md @@ -43,7 +43,7 @@ reviewer IDs F1 to F4 do not collide with an existing audit ID and are kept. Round 2 is da2ce7's review `5639895525` (submitted 2026-10-24 20:39 UTC), a post-merge follow-up because the pull request merged at 20:45 UTC before it was processed. Its reviewer IDs F5 to F9 do not collide and are kept. Its -fixes are on the open follow-up pull request #2667, so the rows are `FOLLOW_UP` and `OPEN` until it merges. +fixes were first on the open follow-up pull request #2667 (the rows were `FOLLOW_UP` and `OPEN`) and merged through it, so the rows are now `FIXED` and `RESOLVED`. | Finding ID | Review finding reference | Author class | Severity | Category | Relationship | Disposition | Thread state | | ---------- | ------------------------ | ------------ | -------- | -------- | ------------ | ----------- | ------------ | @@ -51,11 +51,11 @@ fixes are on the open follow-up pull request #2667, so the rows are `FOLLOW_UP` | F2 | review-finding:pr-2638-f2 | Copilot | Minor | documentation | ORIGINAL | FIXED | RESOLVED | | F3 | review-finding:pr-2638-f3 | Copilot | Minor | documentation | ORIGINAL | FIXED | RESOLVED | | F4 | review-finding:pr-2638-f4 | Copilot | Minor | correctness | ORIGINAL | FIXED | RESOLVED | -| F5 | review-finding:pr-2638-f5 | Human | Minor | documentation | ORIGINAL | FOLLOW_UP | OPEN | -| F6 | review-finding:pr-2638-f6 | Human | Nit | correctness | ORIGINAL | FOLLOW_UP | OPEN | -| F7 | review-finding:pr-2638-f7 | Human | Nit | documentation | ORIGINAL | FOLLOW_UP | OPEN | -| F8 | review-finding:pr-2638-f8 | Human | Nit | documentation | ORIGINAL | FOLLOW_UP | OPEN | -| F9 | review-finding:pr-2638-f9 | Human | Nit | documentation | ORIGINAL | FOLLOW_UP | OPEN | +| F5 | review-finding:pr-2638-f5 | Human | Minor | documentation | ORIGINAL | FIXED | RESOLVED | +| F6 | review-finding:pr-2638-f6 | Human | Nit | correctness | ORIGINAL | FIXED | RESOLVED | +| F7 | review-finding:pr-2638-f7 | Human | Nit | documentation | ORIGINAL | FIXED | RESOLVED | +| F8 | review-finding:pr-2638-f8 | Human | Nit | documentation | ORIGINAL | FIXED | RESOLVED | +| F9 | review-finding:pr-2638-f9 | Human | Nit | documentation | ORIGINAL | FIXED | RESOLVED | ## Finding Details @@ -118,7 +118,7 @@ fixes are on the open follow-up pull request #2667, so the rows are `FOLLOW_UP` - Reviewer finding ID: N/A - Source URL: https://github.com/torrust/torrust-tracker/pull/2638#discussion_r4368445264 - Concern: The spec and EPIC progress-log entries were stamped 20:05 and 20:20 UTC (the forge says the issue was created at 19:57:14 UTC and the PR opened at 20:00 UTC), the EPIC entry and the stamps came from a commit authored at 19:59 UTC, and `last-updated-utc` was not bumped after four later commits that changed the files without a log entry. -- Solution: The entries are re-stamped with the forge times, the review round and the merge are logged, and `last-updated-utc` is bumped in both files. Fixed on the follow-up branch of PR #2667; not on `develop` until that pull request merges. +- Solution: The entries are re-stamped with the forge times, the review round and the merge are logged, and `last-updated-utc` is bumped in both files. Merged through the follow-up pull request #2667. - Current-tree verification: Read the progress logs of `ISSUE.md` and `EPIC.md` against `gh issue view 2637 --json createdAt` (19:57:14Z) and `gh pr view 2638 --json createdAt` (20:00:00Z). - Resolution reference: docs(issues): [#2637] stamp the spec and EPIC log entries with the event times - Follow-up PR URL: https://github.com/torrust/torrust-tracker/pull/2667 @@ -131,7 +131,7 @@ fixes are on the open follow-up pull request #2667, so the rows are `FOLLOW_UP` - Reviewer finding ID: N/A - Source URL: https://github.com/torrust/torrust-tracker/pull/2638#discussion_r4368445267 - Concern: `coverage-evidence.md` named `routes.rs:136` as the handler that answers `408`; at the measured commit that line closes the `on_failure` closure, and the `HandleErrorLayer` closure that answers `408` is at line 144, which the unit-only report does not list. -- Solution: The group is split: lines 122-129 and 136 are the logging `on_failure` closure, and line 144 is named as the `408` handler, with the reason the report does not list it. Fixed on the follow-up branch of PR #2667; not on `develop` until that pull request merges. +- Solution: The group is split: lines 122-129 and 136 are the logging `on_failure` closure, and line 144 is named as the `408` handler, with the reason the report does not list it. Merged through the follow-up pull request #2667. - Current-tree verification: Read `packages/axum-rest-api-server/src/routes.rs` lines 136 and 144 and the two rows of `coverage-evidence.md`. - Resolution reference: docs(issues): [#2637] name the 408 timeout handler line in the coverage evidence - Follow-up PR URL: https://github.com/torrust/torrust-tracker/pull/2667 @@ -144,7 +144,7 @@ fixes are on the open follow-up pull request #2667, so the rows are `FOLLOW_UP` - Reviewer finding ID: N/A - Source URL: https://github.com/torrust/torrust-tracker/pull/2638#discussion_r4368445269 - Concern: Row 4 of the inventory (`v1/context/health_check/handlers.rs`) showed `1 (integration)` in the column that counts in-file `#[test]` functions; the file has none. -- Solution: The cell reads `0 (1 integration)`, as rows 6, 9, 12 and 15 do. Fixed on the follow-up branch of PR #2667; not on `develop` until that pull request merges. +- Solution: The cell reads `0 (1 integration)`, as rows 6, 9, 12 and 15 do. Merged through the follow-up pull request #2667. - Current-tree verification: Read row 4 of the inventory; `grep -c '#\[test\]\|#\[tokio::test\]'` on the file gives 0. - Resolution reference: docs(issues): [#2637] write the health check test count as zero in-file - Follow-up PR URL: https://github.com/torrust/torrust-tracker/pull/2667 @@ -157,7 +157,7 @@ fixes are on the open follow-up pull request #2667, so the rows are `FOLLOW_UP` - Reviewer finding ID: N/A - Source URL: https://github.com/torrust/torrust-tracker/pull/2638#discussion_r4368445271 - Concern: The property-based testing row of `coverage-evidence.md` still said `the route gating by configuration` after the F4 fix of this audit said every such statement had been changed. -- Solution: The row now says `capability gating`; the sweep covered `ISSUE.md` and `coverage-evidence.md` and `grep -rn 'route gating'` over the issue folder finds nothing. Fixed on the follow-up branch of PR #2667; not on `develop` until that pull request merges. +- Solution: The row now says `capability gating`; the sweep covered `ISSUE.md` and `coverage-evidence.md` and `grep -rn 'route gating'` over the issue folder finds nothing. Merged through the follow-up pull request #2667. - Current-tree verification: `grep -rn 'route gating' docs/issues/open/2637-1347-add-tests-axum-rest-api-server` returned no match after the change. - Resolution reference: docs(issues): [#2637] finish the capability gating wording in the technique assessment - Follow-up PR URL: https://github.com/torrust/torrust-tracker/pull/2667 @@ -170,7 +170,7 @@ fixes are on the open follow-up pull request #2667, so the rows are `FOLLOW_UP` - Reviewer finding ID: N/A - Source URL: https://github.com/torrust/torrust-tracker/pull/2638#discussion_r4368445273 - Concern: The Processing Log entry of 20:18 UTC said `No review from the maintainer da2ce7`; da2ce7 was the requested reviewer, and the maintainer is the PR author. -- Solution: The log is append-only, so a correction entry names the rewritten wording and the 20:18 UTC entry is left as written. Fixed on the follow-up branch of PR #2667; not on `develop` until that pull request merges. +- Solution: The log is append-only, so a correction entry names the rewritten wording and the 20:18 UTC entry is left as written. Merged through the follow-up pull request #2667. - Current-tree verification: Read the Processing Log of this audit after the change. - Resolution reference: docs(pr-reviews): correct the role of da2ce7 in the PR 2638 audit log - Follow-up PR URL: https://github.com/torrust/torrust-tracker/pull/2667 @@ -183,6 +183,7 @@ fixes are on the open follow-up pull request #2667, so the rows are `FOLLOW_UP` - 2026-10-24 20:24 UTC - Replied to the four threads (reply times 20:24:17 to 20:24:24 UTC) and resolved them; each resolve mutation returned `isResolved: true`. - 2026-10-24 20:53 UTC - Correction to the 20:18 UTC entry: it called da2ce7 "the maintainer", but da2ce7 is the requested reviewer (the maintainer is the PR author, josecelano), as da2ce7's finding F9 states. The 20:18 UTC entry is left as written. - 2026-10-27 07:32 UTC - Recorded late: da2ce7's review `5639895525` was submitted at 2026-10-24 20:39 UTC with five inline findings (F5 to F9), after the Copilot round above and before the pull request merged at 20:45 UTC; the five findings were unprocessed at the merge. Triage was read-only first and the maintainer chose to fold the fixes into the S1 branch. The five replies on the merged threads (07:32:50 to 07:32:59 UTC) carry the `FOLLOW_UP` disposition, the follow-up pull request #2667 and the approval comment created at 07:32:25 UTC. The threads stay open until #2667 merges. +- 2026-10-27 14:56 UTC - The follow-up pull request #2667 was merged at 11:47 UTC (merge commit subject `Merge torrust/torrust-tracker#2667: test(axum-rest-api-server): [#2637] cover the authentication middleware and the shared responses`). Re-checked the five fixes in the `develop` tree, replied to each thread (14:55:46 to 14:55:55 UTC) and resolved all five (each mutation returned `isResolved: true`); F5 to F9 are `FIXED` and `RESOLVED`. This update travels with the next slice pull request, because the merged branch cannot carry it. ## Completion Rules From 4efc7d3ea52e0a386a467d8d8e34203ed2aae6b4 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 27 Oct 2026 14:59:28 +0000 Subject: [PATCH 02/17] docs(issues): [#2637] start S2 and add the v1 route composition 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> --- .../1347-overhaul-packages-testing/EPIC.md | 4 +- .../ISSUE.md | 19 ++- .../test-refactor-plans/README.md | 1 + .../test-refactor-plans/v1-routes-tests.md | 120 ++++++++++++++++++ 4 files changed, 136 insertions(+), 8 deletions(-) create mode 100644 docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md diff --git a/docs/issues/open/1347-overhaul-packages-testing/EPIC.md b/docs/issues/open/1347-overhaul-packages-testing/EPIC.md index 6ec31eb26..baabc5251 100644 --- a/docs/issues/open/1347-overhaul-packages-testing/EPIC.md +++ b/docs/issues/open/1347-overhaul-packages-testing/EPIC.md @@ -6,7 +6,7 @@ epic: null github-issue: 1347 spec-path: docs/issues/open/1347-overhaul-packages-testing/EPIC.md epic-owner: josecelano -last-updated-utc: "2026-10-27 08:07" +last-updated-utc: "2026-10-27 14:58" semantic-links: skill-links: - create-issue @@ -95,7 +95,7 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. | 7 | #2283 - Increase UDP server package coverage | `docs/issues/closed/2283-1347-increase-udp-server-package-coverage/ISSUE.md` | DONE | Every package source file processed through a reviewed per-file plan; unit-only lines 96.18% → 98.18%; bounded mutation sample and manual verification recorded; merged via PR #2381. | | 8 | #2301 - Review package coverage rollout | `docs/issues/open/2301-1347-review-package-coverage-rollout/ISSUE.md` | TODO | After the EPIC completes, review real rollout evidence before retaining, revising, or proposing a required coverage check. | | 9 | #2361 - Keep the report-only package coverage summary from failing on a discovery failure | `docs/issues/open/2361-1347-package-coverage-summary-discovery-failure/ISSUE.md` | TODO | Bug follow-up of #2347 (`review-finding:pr-2293-f1`): the #2222 summary check goes red with a JSON parse error when discovery fails. | -| 10 | #2637 - Add tests to the axum-rest-api-server package | `docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md` | IN_PROGRESS | S0 (spec and T1 baseline) merged through #2638; S1 (authentication and shared responses) is in review in #2667; slices S2-S6 follow. Replaces the former draft that was misfiled under #1349. | +| 10 | #2637 - Add tests to the axum-rest-api-server package | `docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md` | IN_PROGRESS | S0 (spec and T1 baseline) merged through #2638; S1 (authentication and shared responses) merged through #2667; S2 (routes) started; S3-S6 follow. Replaces the former draft that was misfiled under #1349. | | 11 | Additional package-testing subissues | Create a folder-style spec when a concrete package need is identified | TODO | Permitted but not required upfront; retain scope in this EPIC. | ## Package Coverage Tracking diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index 9449719a6..9b4253720 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -7,9 +7,9 @@ priority: p2 epic: 1347 github-issue: 2637 spec-path: docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md -branch: "2637-1347-add-tests-axum-rest-api-server-auth" -related-pr: 2667 -last-updated-utc: "2026-10-27 08:07" +branch: "2637-1347-add-tests-axum-rest-api-server-routes" +related-pr: null +last-updated-utc: "2026-10-27 14:58" semantic-links: skill-links: - create-issue @@ -403,8 +403,8 @@ ledger order inside each slice is still confirmed with the first plan (T3). | Slice | Content | Ledger rows | Branch | PR | Status | | --- | --- | --- | --- | --- | --- | | S0 | This specification, the T1 coverage evidence and inventory, and the EPIC registration with the baseline | - | `2637-1347-add-tests-axum-rest-api-server-spec` | #2638 | DONE | -| S1 | T2-T3, then authentication and the shared response contract (ranks 1 and 3) | 1, 5 | `2637-1347-add-tests-axum-rest-api-server-auth` | #2667 | IN_PROGRESS | -| S2 | Route composition, transport layers and the health check (rank 2) | 2-4 | `2637-1347-add-tests-axum-rest-api-server-routes` | TBD | PENDING | +| S1 | T2-T3, then authentication and the shared response contract (ranks 1 and 3) | 1, 5 | `2637-1347-add-tests-axum-rest-api-server-auth` | #2667 | DONE | +| S2 | Route composition, transport layers and the health check (rank 2) | 2-4 | `2637-1347-add-tests-axum-rest-api-server-routes` | TBD | IN_PROGRESS | | S3 | State-changing contexts: auth keys and whitelist (rank 4) | 6-11 | `2637-1347-add-tests-axum-rest-api-server-commands` | TBD | PENDING | | S4 | Read-only contexts: torrents and statistics (rank 5) | 12-17 | `2637-1347-add-tests-axum-rest-api-server-queries` | TBD | PENDING | | S5 | Server lifecycle (rank 6) | 18 | `2637-1347-add-tests-axum-rest-api-server-lifecycle` | TBD | PENDING | @@ -533,7 +533,7 @@ declaration-only rows have no executable entries. | Order | Source module | Rank | Slice | Unit-only lines | Tests | Initial hypothesis | State | Plan | | ---: | --- | ---: | --- | ---: | ---: | --- | --- | --- | | 1 | `v1/middlewares/auth.rs` | 1 | S1 | 71 / 103 (68.93%) | 4 | Property candidate | DONE | [v1-middlewares-auth-tests.md](test-refactor-plans/v1-middlewares-auth-tests.md) (done; 142 / 142 lines after, 27 test cases, 8 mutants killed, property test declined) | -| 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | PENDING | - | +| 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | IN_PROGRESS | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (proposed) | | 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | PENDING | - | | 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | PENDING | - | | 5 | `v1/responses.rs` | 3 | S1 | 8 / 40 (20.00%) | 0 | Test candidate | DONE | [v1-responses-tests.md](test-refactor-plans/v1-responses-tests.md) (done; 66 / 66 lines after, 8 test cases, 8 mutants killed) | @@ -658,6 +658,13 @@ once. If the slicing changes, the Slice column changes with it. again: unit-only lines 921 / 1,305 (70.57%), aggregate 1,161 / 1,305 (88.97%), integration-only 674 / 908 (74.23%); 46 unit tests. Updated the EPIC "Latest" values. - 2026-10-27 08:07 UTC - GitHub Copilot - Recorded S1 pull request #2667 () with `Related to #2637`, opened at 07:32 UTC after both independent reviews; recorded its number in the frontmatter and the slice table after Copilot's review (finding F1 of the PR #2667 audit). +- 2026-10-27 14:58 UTC - GitHub Copilot - S1 merged through #2667 (11:47 UTC, with your ACK at 10:23 UTC; + Copilot's one nit F1 was fixed before the merge). Closed the loop on the late #2638 findings F5 to F9: the + fixes are on `develop`, I replied to and resolved the five threads (14:55 UTC) and set their audit rows to + `FIXED`/`RESOLVED` in the first commit of this branch. Started S2 on branch + `2637-1347-add-tests-axum-rest-api-server-routes` from `develop` at the merge of #2672. The package is + byte-identical to the S1 head, so the S1 measurement is the S2 baseline (unit-only lines 921 / 1,305, + 70.57%). Row 2 (`v1/routes.rs`) is `IN_PROGRESS`; its plan is proposed and awaits the maintainer. ## Acceptance Criteria diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md index f13aa3eaa..93ee41569 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md @@ -27,6 +27,7 @@ approves it. A group of declaration-only files may share one plan. | ---: | --- | --- | --- | | 1 | [v1-middlewares-auth-tests.md](v1-middlewares-auth-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-26 | | 2 | [v1-responses-tests.md](v1-responses-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-27 | +| 3 | [v1-routes-tests.md](v1-routes-tests.md) | PROPOSED | Awaiting maintainer approval | Add a row when a plan is created; the remaining files and their order are in the ledger. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md new file mode 100644 index 000000000..e419cdd17 --- /dev/null +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md @@ -0,0 +1,120 @@ +--- +doc-type: file-test-plan +issue: 2637 +package: torrust-tracker-axum-rest-api-server +target-file: packages/axum-rest-api-server/src/v1/routes.rs +status: proposed +semantic-links: + related-artifacts: + - packages/axum-rest-api-server/src/v1/routes.rs + - docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md +--- + +# v1 Route Composition File Test Plan + +Follow the shared [guidance](README.md). This plan covers only `packages/axum-rest-api-server/src/v1/routes.rs`, +ledger row 2 (rank 2), the first file of S2. + +## Current State + +- **Unit-only coverage:** 27 / 45 lines (60.00%), 51 / 73 regions, 1 / 3 functions + ([coverage evidence](../coverage-evidence.md#slice-s1-measurement); the package is identical to the S1 head). + The covered lines run through the server and test-environment unit tests, which start the real router with the + default (public, not listed) configuration, so only the "capability off" outcome runs. No test is in the file. +- **Existing tests:** none in the file. The 58 integration tests exercise every endpoint over HTTP and are kept. +- **Module-owned decision:** `add` registers the four contexts on one router. Stats and torrent services are + always present. The auth-key service exists only when `core.private` is set **and** persistence is composed; + the whitelist service only when `core.listed` is set **and** persistence is composed. The auth-key and + whitelist routes are registered in every case (see the capability-gating wording in the spec); an absent + service makes their handlers answer `409`. +- **T1 hypothesis:** test candidate. + +## Current Tests Review + +No tests in the file, so R1 is skipped. + +## Coverage And Decision Analysis + +| Behavior group (lines at the S1 head) | Classification | Decision | +| --- | --- | --- | +| Auth-key service built in private mode with persistence (26-33) | 3A unit | R2: call `add` and send one request to a key route for each state of (`private`, persistence). | +| Whitelist service built in listed mode with persistence (56-63) | 3A unit | R2: the same for (`listed`, persistence). | +| Stats and torrent services are always present | 3A unit | R2: with the least capable composition (public, no persistence) both read endpoints answer `200`. | +| Every route of the four contexts is registered | 3A unit | R2: a table of method and path pairs; a registered route never answers `404`. This also pins the paths of the four `*/routes.rs` files (ledger rows 8, 11, 14, 17). | +| What the services do (key validity, persistence, DTOs) | Collaborator-owned | `rest-api-application`, `rest-api-runtime-adapter` and `tracker-core`; asserted only as "not the disabled `409`". | + +### Boundary And Arrange + +The test calls `v1::routes::add("/api", Router::new(), &container)` and sends requests with +`tower::ServiceExt::oneshot`. It does **not** go through `routes::router`, so the authentication layer, request +id, compression and timeout (row 3, `routes.rs`) are not involved. The container is built by the package's +`TrackerHttpApiCoreContainer::initialize` from a `torrust-tracker-test-helpers` configuration whose `core.private`, +`core.listed` and `core.database` are set by the test. `ephemeral()` already gives each call its own temporary +SQLite file, and `core.database = None` composes no persistence. + +### Cases For R2 (literal expectations) + +| State | Request | Expected | +| --- | --- | --- | +| `private`, persistence | `POST /api/v1/key/60` | `200` (a generated key; only the status is asserted) | +| not `private`, persistence | `POST /api/v1/key/60` | `409`, `application/json`, `{"status":"err","reason":"private capability is disabled by configuration"}` | +| `private`, no persistence | `POST /api/v1/key/60` | the same `409` | +| `listed`, persistence | `GET /api/v1/whitelist/reload` | `200` | +| not `listed`, persistence | `GET /api/v1/whitelist/reload` | `409`, `{"status":"err","reason":"listed capability is disabled by configuration"}` | +| `listed`, no persistence | `GET /api/v1/whitelist/reload` | the same `409` | +| public, no persistence | `GET /api/v1/stats` and `GET /api/v1/torrents` | `200` | + +Route registration table (the least capable composition, so a gated route answers `409` and an open one `200` or +a handler error, never `404`): `POST /key/60`, `DELETE /key/abc`, `POST /keys` (empty body), `GET /keys/reload`, +`POST` and `DELETE /whitelist/<40 hex>`, `GET /whitelist/reload`, `GET /stats`, `GET /metrics`, +`GET /torrents`, `GET /torrent/<40 hex>`. Expected: status is not `404` and not `405`. + +## Property-Test Decision + +Not flagged and not selected: the outcome is a small enumerable policy (two booleans and a persistence flag), +better served by an `rstest` table. + +## Dependencies + +None new. `rstest` and `tower` (`util`) exist; `torrust-tracker-test-helpers` is already a dev-dependency. + +## Steps + +| ID | Status | Work | Completion evidence | +| --- | --- | --- | --- | +| R1 | SKIPPED | No existing tests. | None. | +| R2 | TODO | Add the in-file test module with the gating and registration tables and a helper that builds the container. | Focused tests, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | +| R3 | TODO | Integration increment: `none` expected. | The 58 integration tests keep the wire contract. | +| R4 | TODO | Re-run unit-only coverage; record before and after. | Results below and `coverage-evidence.md`. | + +## Named Mutations + +| ID | Site | Temporary change | Expected killer | +| --- | --- | --- | --- | +| G1 | auth-key gate | Remove the `private` condition (always build the service when persistence exists). | "not `private`, persistence" | +| G2 | auth-key gate | Make the `private` branch return `None` (the service is never built). | "`private`, persistence" | +| G3 | whitelist gate | Swap `listed` for `private`. | the two `listed` cases | +| G4 | whitelist gate | Always `None`. | "`listed`, persistence" | +| G5 | auth-key routes | Do not register the auth-key routes when the service is absent. | the "no persistence" `409` cases and the route table | +| G6 | stats service | Skip `stats::routes::add`. | the `stats` cases and the route table | + +## Maintainer Questions + +1. **Shared test container helper.** Row 3 (`routes.rs`) and later `server.rs` (S5) need the same container + builder, and `server.rs` already has a private copy inside its test module. Proposal: add one + `#[cfg(test)] pub(crate)` module in this package (for example `src/test_support.rs`) with + `api_container(&Configuration) -> Arc`, used from here, row 3 and S5. It does + not depend on `testing::environment` (which #2482 gates). Alternative: a private helper in each test module. + Which do you prefer? +2. **Persistence "none" case.** It relies on `TrackerCoreContainer::initialize_from` accepting + `core.database = None` with `private` or `listed` set. If a configuration rule forbids that combination in + production, the case still protects the route's behavior, because `add` only reads the composed + persistence. Confirm the case stays. + +## Prose-First AAA Review + +To be written for every added test before the completed-file review. + +## Results + +Pending. From 409476601a580fd07973bb90a49ac5af3bc5ff88 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 12:20:21 +0000 Subject: [PATCH 03/17] docs(issues): [#2637] approve the v1 route plan and link its two 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> --- .../ISSUE.md | 10 +++- .../lessons.md | 2 + .../test-refactor-plans/README.md | 2 +- .../test-refactor-plans/v1-routes-tests.md | 55 +++++++++++++------ 4 files changed, 49 insertions(+), 20 deletions(-) diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index 9b4253720..78e54ed74 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -9,7 +9,7 @@ github-issue: 2637 spec-path: docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md branch: "2637-1347-add-tests-axum-rest-api-server-routes" related-pr: null -last-updated-utc: "2026-10-27 14:58" +last-updated-utc: "2026-10-28 12:20" semantic-links: skill-links: - create-issue @@ -533,7 +533,7 @@ declaration-only rows have no executable entries. | Order | Source module | Rank | Slice | Unit-only lines | Tests | Initial hypothesis | State | Plan | | ---: | --- | ---: | --- | ---: | ---: | --- | --- | --- | | 1 | `v1/middlewares/auth.rs` | 1 | S1 | 71 / 103 (68.93%) | 4 | Property candidate | DONE | [v1-middlewares-auth-tests.md](test-refactor-plans/v1-middlewares-auth-tests.md) (done; 142 / 142 lines after, 27 test cases, 8 mutants killed, property test declined) | -| 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | IN_PROGRESS | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (proposed) | +| 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | IN_PROGRESS | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (approved 2026-10-28) | | 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | PENDING | - | | 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | PENDING | - | | 5 | `v1/responses.rs` | 3 | S1 | 8 / 40 (20.00%) | 0 | Test candidate | DONE | [v1-responses-tests.md](test-refactor-plans/v1-responses-tests.md) (done; 66 / 66 lines after, 8 test cases, 8 mutants killed) | @@ -665,6 +665,12 @@ once. If the slicing changes, the Slice column changes with it. `2637-1347-add-tests-axum-rest-api-server-routes` from `develop` at the merge of #2672. The package is byte-identical to the S1 head, so the S1 measurement is the S2 baseline (unit-only lines 921 / 1,305, 70.57%). Row 2 (`v1/routes.rs`) is `IN_PROGRESS`; its plan is proposed and awaits the maintainer. +- 2026-10-28 12:20 UTC - Jose Celano - Approved the row 2 plan (`v1/routes.rs`) with these decisions: the + crate-private container helper is a `#[cfg(test)] mod test_support` (ADR `20261021190318`, rule C; `src/testing/` + is for code shared outside the crate); the cases with `private` or `listed` set but no persistence are kept as the + second line of defense, linked to the first line (`validate_persistence_requirements` in + `src/bootstrap/persistence.rs`, called by `setup` in `src/bootstrap/app.rs`); the design question (making that + state unrepresentable) and a stale module comment are recorded in `lessons.md` only. ## Acceptance Criteria diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md index fe783f25b..bb05aaef4 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md @@ -16,6 +16,8 @@ Candidate lessons and follow-ups collected while working on Issue #2637. Promoti | --- | --- | --- | | [Auth middleware plan](test-refactor-plans/v1-middlewares-auth-tests.md) | Decide whether a non-UTF-8 `Authorization` header should be rejected like another scheme or documented as ignored. Fixed as #2653 (merged through #2656; spec in `docs/issues/closed/2653-rest-api-auth-ignores-unreadable-authorization-header/`): the header is rejected as `unknown token provided`. | A throwaway test (reverted) showed `Bearer \xff` with a valid `token` query answers `200`, while `Basic ` answers `500 unknown token provided`. The #2653 test pins the fixed behavior; this plan keeps it in its table. | | `lib.rs` crate documentation | Correct `[http_api.tsl_config]` to `[http_api.tls_config]` in the three places of the crate documentation (found in the review of #2638, finding F1). | The v3 `HttpApi` key is `tls_config` and the type denies unknown fields. Planned for the `lib.rs` plan (S6). | +| [Routes plan](test-refactor-plans/v1-routes-tests.md) | Design question: make the state "private or listed set but no persistence" unrepresentable. `TrackerCoreContainer` holds `core_config.private` and `persistence: Option` as independent fields, and `v1/routes.rs` re-derives service availability from them, which ADR `20260825193119` (agreement 2) says route handlers should not duplicate. Composing the capabilities once (for example `Option` handles built where the database check happens) would remove the combination. It touches `tracker-core` and `rest-api-runtime-adapter`, so it is outside #2637. Maintainer decision 2026-10-28: record here only for now. | The first line of defense is `validate_persistence_requirements` in `src/bootstrap/persistence.rs`, called by `setup` in `src/bootstrap/app.rs`; the second is `v1/routes.rs`, protected by the plan's no-persistence cases. | +| `src/bootstrap/persistence.rs` module comment | Fix the stale comment: it says the check is "intentionally not called" while the runtime uses v2 configuration, but `setup` in `src/bootstrap/app.rs` calls it. Not a behavior bug and outside the package under test. | `grep -n validate_persistence_requirements src/bootstrap/app.rs` finds the call. | ## Testing Lessons diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md index 93ee41569..9bd055062 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md @@ -27,7 +27,7 @@ approves it. A group of declaration-only files may share one plan. | ---: | --- | --- | --- | | 1 | [v1-middlewares-auth-tests.md](v1-middlewares-auth-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-26 | | 2 | [v1-responses-tests.md](v1-responses-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-27 | -| 3 | [v1-routes-tests.md](v1-routes-tests.md) | PROPOSED | Awaiting maintainer approval | +| 3 | [v1-routes-tests.md](v1-routes-tests.md) | APPROVED | Approved 2026-10-28 | Add a row when a plan is created; the remaining files and their order are in the ledger. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md index e419cdd17..e790cb7f5 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md @@ -3,10 +3,14 @@ doc-type: file-test-plan issue: 2637 package: torrust-tracker-axum-rest-api-server target-file: packages/axum-rest-api-server/src/v1/routes.rs -status: proposed +status: approved semantic-links: related-artifacts: - packages/axum-rest-api-server/src/v1/routes.rs + - src/bootstrap/persistence.rs + - src/bootstrap/app.rs + - docs/adrs/20260825193119_make_persistence_an_optional_application_composition_capability.md + - docs/adrs/20261021190318_share_workspace_test_support_through_testing_features.md - docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md --- @@ -43,12 +47,30 @@ No tests in the file, so R1 is skipped. | Every route of the four contexts is registered | 3A unit | R2: a table of method and path pairs; a registered route never answers `404`. This also pins the paths of the four `*/routes.rs` files (ledger rows 8, 11, 14, 17). | | What the services do (key validity, persistence, DTOs) | Collaborator-owned | `rest-api-application`, `rest-api-runtime-adapter` and `tracker-core`; asserted only as "not the disabled `409`". | +### Two Lines Of Defense + +The "private or listed set but no persistence" state is guarded twice, and this plan links both: + +- **First line (startup, owned by the root crate):** `validate_persistence_requirements` in + `src/bootstrap/persistence.rs` rejects `core.listed`, `core.private` and persistent completed statistics + without `[core.database]`, and `setup` in `src/bootstrap/app.rs` calls it before any container is built + (ADR `20260825193119`, agreement 3). Its own tests are in those two files. +- **Second line (this file):** `v1::routes::add` receives a container whose `core_config.private` and + `persistence` are independent fields, so the combination is representable. The handler treats it as + "capability off" and answers `409` instead of building a service or panicking. The "no persistence" cases below + protect that second line. They are defensive: a real tracker never reaches them, because the first line stops + startup. The decision to keep them is the maintainer's (2026-10-28): when the contract of a layer allows a state, + the layer's defense is tested even if an earlier layer validates. + +Whether the state could be made unrepresentable in the types is a design question outside this issue; it is +recorded in `lessons.md`. + ### Boundary And Arrange The test calls `v1::routes::add("/api", Router::new(), &container)` and sends requests with `tower::ServiceExt::oneshot`. It does **not** go through `routes::router`, so the authentication layer, request id, compression and timeout (row 3, `routes.rs`) are not involved. The container is built by the package's -`TrackerHttpApiCoreContainer::initialize` from a `torrust-tracker-test-helpers` configuration whose `core.private`, +`TrackerHttpApiCoreContainer::initialize` (through the crate-private helper below) from a `torrust-tracker-test-helpers` configuration whose `core.private`, `core.listed` and `core.database` are set by the test. `ephemeral()` already gives each call its own temporary SQLite file, and `core.database = None` composes no persistence. @@ -58,10 +80,10 @@ SQLite file, and `core.database = None` composes no persistence. | --- | --- | --- | | `private`, persistence | `POST /api/v1/key/60` | `200` (a generated key; only the status is asserted) | | not `private`, persistence | `POST /api/v1/key/60` | `409`, `application/json`, `{"status":"err","reason":"private capability is disabled by configuration"}` | -| `private`, no persistence | `POST /api/v1/key/60` | the same `409` | +| `private`, no persistence (second line) | `POST /api/v1/key/60` | the same `409` | | `listed`, persistence | `GET /api/v1/whitelist/reload` | `200` | | not `listed`, persistence | `GET /api/v1/whitelist/reload` | `409`, `{"status":"err","reason":"listed capability is disabled by configuration"}` | -| `listed`, no persistence | `GET /api/v1/whitelist/reload` | the same `409` | +| `listed`, no persistence (second line) | `GET /api/v1/whitelist/reload` | the same `409` | | public, no persistence | `GET /api/v1/stats` and `GET /api/v1/torrents` | `200` | Route registration table (the least capable composition, so a gated route answers `409` and an open one `200` or @@ -83,7 +105,7 @@ None new. `rstest` and `tower` (`util`) exist; `torrust-tracker-test-helpers` is | ID | Status | Work | Completion evidence | | --- | --- | --- | --- | | R1 | SKIPPED | No existing tests. | None. | -| R2 | TODO | Add the in-file test module with the gating and registration tables and a helper that builds the container. | Focused tests, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | +| R2 | TODO | Add `src/test_support.rs` (the crate-private container helper) and the in-file test module with the gating and registration tables. | Focused tests, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | | R3 | TODO | Integration increment: `none` expected. | The 58 integration tests keep the wire contract. | | R4 | TODO | Re-run unit-only coverage; record before and after. | Results below and `coverage-evidence.md`. | @@ -98,18 +120,17 @@ None new. `rstest` and `tower` (`util`) exist; `torrust-tracker-test-helpers` is | G5 | auth-key routes | Do not register the auth-key routes when the service is absent. | the "no persistence" `409` cases and the route table | | G6 | stats service | Skip `stats::routes::add`. | the `stats` cases and the route table | -## Maintainer Questions - -1. **Shared test container helper.** Row 3 (`routes.rs`) and later `server.rs` (S5) need the same container - builder, and `server.rs` already has a private copy inside its test module. Proposal: add one - `#[cfg(test)] pub(crate)` module in this package (for example `src/test_support.rs`) with - `api_container(&Configuration) -> Arc`, used from here, row 3 and S5. It does - not depend on `testing::environment` (which #2482 gates). Alternative: a private helper in each test module. - Which do you prefer? -2. **Persistence "none" case.** It relies on `TrackerCoreContainer::initialize_from` accepting - `core.database = None` with `private` or `listed` set. If a configuration rule forbids that combination in - production, the case still protects the route's behavior, because `add` only reads the composed - persistence. Confirm the case stays. +## Maintainer Decisions + +Recorded on 2026-10-28. + +1. **Container helper:** a private `#[cfg(test)] mod test_support` in this crate (`src/test_support.rs`), as + rule C of ADR `20261021190318` says for a helper used only by the crate's own unit tests. It is not placed in + `src/testing/`, which holds test support shared outside the crate (rules A and D) and is gated by #2482. It is + reused by `routes.rs` (row 3) and by `server.rs` in S5, whose private copy it replaces then. +2. **Cases with `private` or `listed` set but no persistence:** kept, labelled as the second line of defense and + linked to the first line (above). +3. **Plan approved** with the edits above. ## Prose-First AAA Review From ae4bf720e73dcbf2a032db5624af1cdcd5a5ff16 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 13:23:23 +0000 Subject: [PATCH 04/17] test(axum-rest-api-server): [#2637] cover the v1 route composition 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> --- .../ISSUE.md | 10 +- .../coverage-evidence.md | 4 + .../lessons.md | 1 + .../test-refactor-plans/README.md | 2 +- .../test-refactor-plans/v1-routes-tests.md | 50 +++++- packages/axum-rest-api-server/src/lib.rs | 2 + .../axum-rest-api-server/src/test_support.rs | 50 ++++++ .../axum-rest-api-server/src/v1/routes.rs | 167 ++++++++++++++++++ 8 files changed, 277 insertions(+), 9 deletions(-) create mode 100644 packages/axum-rest-api-server/src/test_support.rs diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index 78e54ed74..058dd43dc 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -9,7 +9,7 @@ github-issue: 2637 spec-path: docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md branch: "2637-1347-add-tests-axum-rest-api-server-routes" related-pr: null -last-updated-utc: "2026-10-28 12:20" +last-updated-utc: "2026-10-28 13:22" semantic-links: skill-links: - create-issue @@ -533,7 +533,7 @@ declaration-only rows have no executable entries. | Order | Source module | Rank | Slice | Unit-only lines | Tests | Initial hypothesis | State | Plan | | ---: | --- | ---: | --- | ---: | ---: | --- | --- | --- | | 1 | `v1/middlewares/auth.rs` | 1 | S1 | 71 / 103 (68.93%) | 4 | Property candidate | DONE | [v1-middlewares-auth-tests.md](test-refactor-plans/v1-middlewares-auth-tests.md) (done; 142 / 142 lines after, 27 test cases, 8 mutants killed, property test declined) | -| 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | IN_PROGRESS | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (approved 2026-10-28) | +| 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | DONE | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (done; 111 / 111 lines after, 19 test cases, 6 mutants killed, private test_support helper) | | 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | PENDING | - | | 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | PENDING | - | | 5 | `v1/responses.rs` | 3 | S1 | 8 / 40 (20.00%) | 0 | Test candidate | DONE | [v1-responses-tests.md](test-refactor-plans/v1-responses-tests.md) (done; 66 / 66 lines after, 8 test cases, 8 mutants killed) | @@ -671,6 +671,12 @@ once. If the slicing changes, the Slice column changes with it. second line of defense, linked to the first line (`validate_persistence_requirements` in `src/bootstrap/persistence.rs`, called by `setup` in `src/bootstrap/app.rs`); the design question (making that state unrepresentable) and a stale module comment are recorded in `lessons.md` only. +- 2026-10-28 13:22 UTC - GitHub Copilot - Jose Celano questioned whether `src/testing/` should hold only shared test + code or all test-only code outside a test module. The repository evidence (history of the `src/environment.rs` + moves, ADR `20261021190318` rule C, `tracker-core`'s crate-root helpers) is in the routes plan. Decision: the helper + stays a private `#[cfg(test)] mod test_support` and its file gets an inner `#![cfg(test)]`; the convention, including + the `test_` file prefix for files that contain only test code, goes to `lessons.md` for a separate ADR + clarification after this issue. ## Acceptance Criteria diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md index 4ec896c3b..fa948d1c6 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md @@ -213,3 +213,7 @@ After both S1 files (`v1/middlewares/auth.rs` and `v1/responses.rs`), on the S1 | Integration-only (`--test integration`) | 58 integration | 674 / 908 (74.23%) | 941 / 1,352 (69.60%) | 97 / 137 (70.80%) | Unit-only lines move 792 / 1,240 (63.87%) at the S1 baseline to 921 / 1,305 (70.57%). Both files count their new test modules, so the denominator grew by 65 lines; the production lines of the two files are now fully covered by unit tests. The integration-only figure is unchanged because the integration binary does not compile the in-file test modules. + +### `v1/routes.rs` (slice S2) + +Unit-only (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): lines 27 / 45 (60.00%) before, 111 / 111 (100.00%) after; regions 51 / 73 before, 174 / 174 after; functions 1 / 3 before, 13 / 13 after. The counts include the in-file test module. The new crate-private `test_support.rs` helper is 31 / 31 lines. Nineteen test cases; six named mutations killed (see [the plan](test-refactor-plans/v1-routes-tests.md)). diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md index bb05aaef4..107f7723d 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md @@ -18,6 +18,7 @@ Candidate lessons and follow-ups collected while working on Issue #2637. Promoti | `lib.rs` crate documentation | Correct `[http_api.tsl_config]` to `[http_api.tls_config]` in the three places of the crate documentation (found in the review of #2638, finding F1). | The v3 `HttpApi` key is `tls_config` and the type denies unknown fields. Planned for the `lib.rs` plan (S6). | | [Routes plan](test-refactor-plans/v1-routes-tests.md) | Design question: make the state "private or listed set but no persistence" unrepresentable. `TrackerCoreContainer` holds `core_config.private` and `persistence: Option` as independent fields, and `v1/routes.rs` re-derives service availability from them, which ADR `20260825193119` (agreement 2) says route handlers should not duplicate. Composing the capabilities once (for example `Option` handles built where the database check happens) would remove the combination. It touches `tracker-core` and `rest-api-runtime-adapter`, so it is outside #2637. Maintainer decision 2026-10-28: record here only for now. | The first line of defense is `validate_persistence_requirements` in `src/bootstrap/persistence.rs`, called by `setup` in `src/bootstrap/app.rs`; the second is `v1/routes.rs`, protected by the plan's no-persistence cases. | | `src/bootstrap/persistence.rs` module comment | Fix the stale comment: it says the check is "intentionally not called" while the runtime uses v2 configuration, but `setup` in `src/bootstrap/app.rs` calls it. Not a behavior bug and outside the package under test. | `grep -n validate_persistence_requirements src/bootstrap/app.rs` finds the call. | +| [Routes plan](test-refactor-plans/v1-routes-tests.md) | Clarify the test-support file conventions in ADR `20261021190318` in a separate small PR after #2637: (1) where a crate-private helper (rule C) lives and how it is marked: a private `#[cfg(test)] mod` with an inner `#![cfg(test)]` in its file, versus a `#[cfg(test)]` submodule under `src/testing/`, and whether `src/testing/` holds only shared, feature-gated code (rule A) or any test code outside a `#[cfg(test)] mod tests`; (2) the naming convention, noted by the maintainer on 2026-10-28, that a file containing only test code is prefixed `test_` (as `test_support.rs`, and `tracker-core`'s `test_helpers.rs`); (3) whether the `tracker-core` crate-root helpers follow it. | The ADR says nothing about the folder of a rule C helper. `git log` shows the moves of `src/environment.rs` into `src/testing/environment.rs` (#1903 and siblings) "marking it explicitly as test-only infrastructure", all of them shared environments. `tracker-core` keeps `test_helpers.rs` and `whitelist/test_helpers.rs` at its crate root with `#[cfg(test)]` content. | ## Testing Lessons diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md index 9bd055062..028cdc993 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md @@ -27,7 +27,7 @@ approves it. A group of declaration-only files may share one plan. | ---: | --- | --- | --- | | 1 | [v1-middlewares-auth-tests.md](v1-middlewares-auth-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-26 | | 2 | [v1-responses-tests.md](v1-responses-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-27 | -| 3 | [v1-routes-tests.md](v1-routes-tests.md) | APPROVED | Approved 2026-10-28 | +| 3 | [v1-routes-tests.md](v1-routes-tests.md) | DONE | Approved 2026-10-28; completed file approved 2026-10-28 | Add a row when a plan is created; the remaining files and their order are in the ledger. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md index e790cb7f5..864baf043 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md @@ -3,7 +3,7 @@ doc-type: file-test-plan issue: 2637 package: torrust-tracker-axum-rest-api-server target-file: packages/axum-rest-api-server/src/v1/routes.rs -status: approved +status: done semantic-links: related-artifacts: - packages/axum-rest-api-server/src/v1/routes.rs @@ -105,9 +105,9 @@ None new. `rstest` and `tower` (`util`) exist; `torrust-tracker-test-helpers` is | ID | Status | Work | Completion evidence | | --- | --- | --- | --- | | R1 | SKIPPED | No existing tests. | None. | -| R2 | TODO | Add `src/test_support.rs` (the crate-private container helper) and the in-file test module with the gating and registration tables. | Focused tests, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | -| R3 | TODO | Integration increment: `none` expected. | The 58 integration tests keep the wire contract. | -| R4 | TODO | Re-run unit-only coverage; record before and after. | Results below and `coverage-evidence.md`. | +| R2 | DONE | Add `src/test_support.rs` (the crate-private container helper) and the in-file test module with the gating and registration tables. | Focused tests, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | +| R3 | DONE | Integration increment: `none` expected. | The 58 integration tests keep the wire contract. | +| R4 | DONE | Re-run unit-only coverage; record before and after. | Results below and `coverage-evidence.md`. | ## Named Mutations @@ -131,11 +131,49 @@ Recorded on 2026-10-28. 2. **Cases with `private` or `listed` set but no persistence:** kept, labelled as the second line of defense and linked to the first line (above). 3. **Plan approved** with the edits above. +4. **Helper location, confirmed 2026-10-28 after questioning the `src/testing/` convention:** the helper stays a + private `#[cfg(test)] mod test_support` at the crate root, and the file also carries the inner attribute + `#![cfg(test)]` so that it states it is test code. Evidence weighed: the `src/environment.rs` to + `src/testing/environment.rs` moves were meant to mark test-only infrastructure but moved shared server + environments; ADR `20261021190318` rule C does not say where a crate-private helper's file lives; and + `tracker-core` keeps `src/test_helpers.rs` at the crate root with only `#[cfg(test)]` content. The + convention is recorded in `lessons.md` for a separate ADR clarification after this issue, together with the + maintainer's note that files containing only test code are prefixed `test_`. ## Prose-First AAA Review -To be written for every added test before the completed-file review. +Temporary prose, written before comparing with the code. + +| Test (cases) | Arrange prose | Act prose | Assert prose | Outcome | +| --- | --- | --- | --- | --- | +| `it_should_serve_the_auth_key_routes_when_the_tracker_is_private_and_has_persistence` | A private tracker with a database. | `POST /api/v1/key/60` through the composed routes. | `200`. | Only the status is asserted: the body is a random key owned by `tracker-core`. Kept. | +| `it_should_answer_conflict_on_the_auth_key_routes_when_the_private_capability_is_not_available` (2) | A public tracker with a database, or a private tracker without one (the second line of defense). | Same request. | `409`, `application/json`, the literal disabled body. | The two states share one outcome, so one table; the case names say which state each is. Kept. | +| `it_should_serve_the_whitelist_routes_when_the_tracker_is_listed_and_has_persistence` | A listed tracker with a database. | `GET /api/v1/whitelist/reload`. | `200`, JSON, `{"status":"ok"}`. | Kept. | +| `it_should_answer_conflict_on_the_whitelist_routes_when_the_listed_capability_is_not_available` (2) | A public tracker with a database, or a listed tracker without one (the second line of defense). | Same request. | `409`, JSON, the literal disabled body. | Kept. | +| `it_should_serve_the_read_routes_even_when_the_tracker_has_no_persistence` (2) | The least capable tracker (public, no database). | `GET` of the statistics and the torrent list. | `200`. | Status only: the bodies are owned by the stats and torrent services. Kept. | +| `it_should_register_every_route_of_the_four_contexts_even_for_the_least_capable_tracker` (11) | The least capable tracker. | One request per method and path of the four contexts. | The status is neither `404` (path not registered) nor `405` (method not registered). | Deliberately weak: it pins the route table, not the behavior of each handler, which is the job of the handler plans (S3 and S4). Kept. | + +Helpers: `private_tracker`, `listed_tracker` and `without_database` name the three configuration states; `call` +builds the container, composes the routes and reads the response, and hides only mechanics. `test_support::api_container` +(in `src/test_support.rs`, rule C of ADR `20261021190318`, with an inner `#![cfg(test)]`) builds the container as the application does. ## Results -Pending. +- **Unit-only coverage** (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): `v1/routes.rs` 27 / 45 + lines (60.00%) before; 111 / 111 lines (100.00%), 174 / 174 regions and 13 / 13 functions after (the counts include + the test module). The new `test_support.rs` is 31 / 31. The four context `routes.rs` files stay at 100% + (their paths and methods are now pinned by the registration table). Package unit-only lines after this file: + 1,142 / 1,402 (81.46%), regions 1,661 / 2,142 (77.54%), functions 157 / 208 (75.48%), 65 unit tests; the + denominator grew by the test module and the helper. +- **Tests:** 19 test cases in six tests (single tests and `rstest` tables of 2, 2, 2 and 11 cases). No + integration test was added or changed. +- **Mutations** (each applied alone, stable toolchain, then reverted; the file was identical to the tested one + afterwards): G1 killed by the public-tracker `409` case, G2 by the private-with-persistence test, G3 and G4 by the + listed-with-persistence test, G5 by six cases (both auth-key `409` cases and the key rows of the registration + table), G6 by three cases (the statistics and metrics registration rows and the statistics read case). The + first G6 attempt left `stats` unused and did not compile under `unused = "deny"`; it was redone as a clone that + is dropped, which compiles. +- **Validation:** `cargo +nightly fmt --all -- --check` (nightly Rust toolchain), `cargo clippy -p + torrust-tracker-axum-rest-api-server --all-targets` (stable), `cargo machete --with-metadata`, `git diff + --check` and `cargo test -p torrust-tracker-axum-rest-api-server` (65 unit and 58 integration tests) pass. +- **Review outcome:** the maintainer approved the completed file on 2026-10-28. diff --git a/packages/axum-rest-api-server/src/lib.rs b/packages/axum-rest-api-server/src/lib.rs index d8880ed1a..6698ffb69 100644 --- a/packages/axum-rest-api-server/src/lib.rs +++ b/packages/axum-rest-api-server/src/lib.rs @@ -155,6 +155,8 @@ //! > errors. For example: `curl "http://127.0.0.1:1212/api/v1/stats?token=MyAccessToken"`. pub mod routes; pub mod server; +#[cfg(test)] +mod test_support; pub mod testing; pub mod v1; diff --git a/packages/axum-rest-api-server/src/test_support.rs b/packages/axum-rest-api-server/src/test_support.rs new file mode 100644 index 000000000..0fe49fbc9 --- /dev/null +++ b/packages/axum-rest-api-server/src/test_support.rs @@ -0,0 +1,50 @@ +#![cfg(test)] +//! Crate-private helpers for the unit tests of this package. +//! +//! This module is compiled only for unit tests and is not part of the public API. Code that other packages' tests +//! use lives in [`crate::testing`] instead. +use std::sync::Arc; + +use torrust_tracker_configuration::v3_0_0::Configuration; +use torrust_tracker_primitives::{ConfigurationInstanceId, ServiceRole}; +use torrust_tracker_rest_api_runtime_adapter::v1::container::TrackerHttpApiCoreContainer; + +/// Builds the REST API container for a configuration, as the application does at startup. +/// +/// # Panics +/// +/// Panics if the configuration enables no HTTP tracker, no UDP tracker or no HTTP API. +pub async fn api_container(configuration: &Configuration) -> Arc { + let core_config = Arc::new(configuration.core.clone()); + let http_tracker_config = Arc::new( + configuration + .http_trackers + .clone() + .expect("the configuration enables an HTTP tracker")[0] + .clone(), + ); + let udp_tracker_config = Arc::new( + configuration + .udp_trackers + .clone() + .expect("the configuration enables a UDP tracker")[0] + .clone(), + ); + let http_api_config = Arc::new( + configuration + .http_api + .clone() + .expect("the configuration enables the HTTP API"), + ); + + TrackerHttpApiCoreContainer::initialize( + &core_config, + &http_tracker_config, + ConfigurationInstanceId::new(ServiceRole::HttpTracker, 0), + &udp_tracker_config, + &configuration.udp_tracker_server, + ConfigurationInstanceId::new(ServiceRole::UdpTracker, 0), + &http_api_config, + ) + .await +} diff --git a/packages/axum-rest-api-server/src/v1/routes.rs b/packages/axum-rest-api-server/src/v1/routes.rs index 55b4382c9..ca15b1a88 100644 --- a/packages/axum-rest-api-server/src/v1/routes.rs +++ b/packages/axum-rest-api-server/src/v1/routes.rs @@ -72,3 +72,170 @@ pub fn add(prefix: &str, router: Router, http_api_container: &Arc Configuration { + let mut configuration = ephemeral(); + configuration.core.private = true; + configuration + } + + fn listed_tracker() -> Configuration { + let mut configuration = ephemeral(); + configuration.core.listed = true; + configuration + } + + fn without_database(mut configuration: Configuration) -> Configuration { + configuration.core.database = None; + configuration + } + + /// Sends one request to the v1 routes composed for the configuration and returns the status, the content type + /// and the body. + async fn call(configuration: &Configuration, method: Method, uri: &str) -> (StatusCode, Option, String) { + let container = api_container(configuration).await; + let router = add("/api", Router::new(), &container); + let request = Request::builder() + .method(method) + .uri(uri) + .body(Body::empty()) + .expect("the request is well formed"); + + let response = router.oneshot(request).await.expect("the router never fails"); + + let status = response.status(); + let content_type = response + .headers() + .get(CONTENT_TYPE) + .map(|value| value.to_str().expect("the content type is text").to_string()); + let body = to_bytes(response.into_body(), usize::MAX) + .await + .expect("the body is readable"); + + ( + status, + content_type, + String::from_utf8(body.to_vec()).expect("the body is text"), + ) + } + + #[tokio::test] + async fn it_should_serve_the_auth_key_routes_when_the_tracker_is_private_and_has_persistence() { + let configuration = private_tracker(); + + let (status, _content_type, _body) = call(&configuration, Method::POST, KEY_PATH).await; + + assert_eq!(status, StatusCode::OK); + } + + #[rstest] + #[case::a_public_tracker_with_persistence(ephemeral())] + #[case::a_private_tracker_without_persistence_as_the_second_line_of_defense(without_database(private_tracker()))] + #[tokio::test] + async fn it_should_answer_conflict_on_the_auth_key_routes_when_the_private_capability_is_not_available( + #[case] configuration: Configuration, + ) { + let (status, content_type, body) = call(&configuration, Method::POST, KEY_PATH).await; + + assert_eq!( + (status, content_type, body), + ( + StatusCode::CONFLICT, + Some("application/json".to_string()), + r#"{"status":"err","reason":"private capability is disabled by configuration"}"#.to_string(), + ) + ); + } + + #[tokio::test] + async fn it_should_serve_the_whitelist_routes_when_the_tracker_is_listed_and_has_persistence() { + let configuration = listed_tracker(); + + let (status, content_type, body) = call(&configuration, Method::GET, WHITELIST_RELOAD_PATH).await; + + assert_eq!( + (status, content_type, body), + ( + StatusCode::OK, + Some("application/json".to_string()), + r#"{"status":"ok"}"#.to_string() + ) + ); + } + + #[rstest] + #[case::a_public_tracker_with_persistence(ephemeral())] + #[case::a_listed_tracker_without_persistence_as_the_second_line_of_defense(without_database(listed_tracker()))] + #[tokio::test] + async fn it_should_answer_conflict_on_the_whitelist_routes_when_the_listed_capability_is_not_available( + #[case] configuration: Configuration, + ) { + let (status, content_type, body) = call(&configuration, Method::GET, WHITELIST_RELOAD_PATH).await; + + assert_eq!( + (status, content_type, body), + ( + StatusCode::CONFLICT, + Some("application/json".to_string()), + r#"{"status":"err","reason":"listed capability is disabled by configuration"}"#.to_string(), + ) + ); + } + + #[rstest] + #[case::the_statistics("/api/v1/stats")] + #[case::the_torrent_list("/api/v1/torrents")] + #[tokio::test] + async fn it_should_serve_the_read_routes_even_when_the_tracker_has_no_persistence(#[case] uri: &str) { + let configuration = without_database(ephemeral()); + + let (status, _content_type, _body) = call(&configuration, Method::GET, uri).await; + + assert_eq!(status, StatusCode::OK); + } + + #[rstest] + #[case::generate_a_key(Method::POST, "/api/v1/key/60")] + #[case::delete_a_key(Method::DELETE, "/api/v1/key/abc")] + #[case::add_a_key(Method::POST, "/api/v1/keys")] + #[case::reload_the_keys(Method::GET, "/api/v1/keys/reload")] + #[case::whitelist_a_torrent(Method::POST, &format!("/api/v1/whitelist/{INFO_HASH}"))] + #[case::remove_a_torrent_from_the_whitelist(Method::DELETE, &format!("/api/v1/whitelist/{INFO_HASH}"))] + #[case::reload_the_whitelist(Method::GET, "/api/v1/whitelist/reload")] + #[case::get_the_statistics(Method::GET, "/api/v1/stats")] + #[case::get_the_metrics(Method::GET, "/api/v1/metrics")] + #[case::get_the_torrents(Method::GET, "/api/v1/torrents")] + #[case::get_a_torrent(Method::GET, &format!("/api/v1/torrent/{INFO_HASH}"))] + #[tokio::test] + async fn it_should_register_every_route_of_the_four_contexts_even_for_the_least_capable_tracker( + #[case] method: Method, + #[case] uri: &str, + ) { + let configuration = without_database(ephemeral()); + + let (status, _content_type, _body) = call(&configuration, method, uri).await; + + assert!( + status != StatusCode::NOT_FOUND && status != StatusCode::METHOD_NOT_ALLOWED, + "the route is not registered: {status}" + ); + } +} From 8e59edcf6e3ab011d2a0a276c02ef58c3ded9803 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 13:25:35 +0000 Subject: [PATCH 05/17] docs(issues): [#2637] add the router layers test plan 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> --- .../ISSUE.md | 9 +- .../test-refactor-plans/README.md | 1 + .../test-refactor-plans/routes-tests.md | 120 ++++++++++++++++++ 3 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index 058dd43dc..bf119aaf5 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -9,7 +9,7 @@ github-issue: 2637 spec-path: docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md branch: "2637-1347-add-tests-axum-rest-api-server-routes" related-pr: null -last-updated-utc: "2026-10-28 13:22" +last-updated-utc: "2026-10-28 13:25" semantic-links: skill-links: - create-issue @@ -534,7 +534,7 @@ declaration-only rows have no executable entries. | ---: | --- | ---: | --- | ---: | ---: | --- | --- | --- | | 1 | `v1/middlewares/auth.rs` | 1 | S1 | 71 / 103 (68.93%) | 4 | Property candidate | DONE | [v1-middlewares-auth-tests.md](test-refactor-plans/v1-middlewares-auth-tests.md) (done; 142 / 142 lines after, 27 test cases, 8 mutants killed, property test declined) | | 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | DONE | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (done; 111 / 111 lines after, 19 test cases, 6 mutants killed, private test_support helper) | -| 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | PENDING | - | +| 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | IN_PROGRESS | [routes-tests.md](test-refactor-plans/routes-tests.md) (proposed) | | 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | PENDING | - | | 5 | `v1/responses.rs` | 3 | S1 | 8 / 40 (20.00%) | 0 | Test candidate | DONE | [v1-responses-tests.md](test-refactor-plans/v1-responses-tests.md) (done; 66 / 66 lines after, 8 test cases, 8 mutants killed) | | 6 | `v1/context/auth_key/handlers.rs` | 4 | S3 | 0 / 56 (0.00%) | 0 | Test candidate | PENDING | - | @@ -677,6 +677,11 @@ once. If the slicing changes, the Slice column changes with it. stays a private `#[cfg(test)] mod test_support` and its file gets an inner `#![cfg(test)]`; the convention, including the `test_` file prefix for files that contain only test code, goes to `lessons.md` for a separate ADR clarification after this issue. +- 2026-10-28 13:25 UTC - GitHub Copilot - S2 row 2 (`v1/routes.rs`) is done: unit-only lines 27 / 45 (60.00%) to 111 / 111 + (100.00%), 19 test cases, six named mutations killed, a crate-private `test_support` helper; the maintainer approved + the completed file (2026-10-28). Proposed the row 3 plan (`routes.rs`: the authentication layer wiring, request id, + compression), with two questions for the maintainer (the `408` timeout has no test seam; asserting log events + needs a new dev-dependency). ## Acceptance Criteria diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md index 028cdc993..6ee1e3052 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md @@ -28,6 +28,7 @@ approves it. A group of declaration-only files may share one plan. | 1 | [v1-middlewares-auth-tests.md](v1-middlewares-auth-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-26 | | 2 | [v1-responses-tests.md](v1-responses-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-27 | | 3 | [v1-routes-tests.md](v1-routes-tests.md) | DONE | Approved 2026-10-28; completed file approved 2026-10-28 | +| 4 | [routes-tests.md](routes-tests.md) | PROPOSED | Awaiting maintainer approval and two answers | Add a row when a plan is created; the remaining files and their order are in the ledger. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md new file mode 100644 index 000000000..22e9ce133 --- /dev/null +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md @@ -0,0 +1,120 @@ +--- +doc-type: file-test-plan +issue: 2637 +package: torrust-tracker-axum-rest-api-server +target-file: packages/axum-rest-api-server/src/routes.rs +status: proposed +semantic-links: + related-artifacts: + - packages/axum-rest-api-server/src/routes.rs + - docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md + - docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md +--- + +# Router Layers File Test Plan + +Follow the shared [guidance](README.md). This plan covers only `packages/axum-rest-api-server/src/routes.rs`, +ledger row 3 (rank 2), the second file of S2. + +## Current State + +- **Unit-only coverage:** 58 / 67 lines (86.57%), 103 / 117 regions, 5 / 8 functions (S2 head, `cargo-llvm-cov 0.6.16`, + stable Rust 1.99.1). The covered lines run through the test-environment unit tests, which start the real router. + Nothing asserts the file's behavior. Uncovered: lines 100 and 102 (the server-error response log arm) and + 124-129 and 136 (the `on_failure` closure). +- **Existing tests:** none in the file. The integration tests exercise the router over HTTP; one asserts that an + authentication failure logs an `ERROR` line with the request id. +- **Module-owned decisions:** `router(container, access_tokens, binding)` composes the v1 routes, puts the + authentication layer on them **before** adding `/api/health_check`, so the health check stays public, then wraps + everything in compression, request-id setting and propagation, request tracing and a 5 s timeout that answers + `408`. The log events are structured and use the `API` target. +- **T1 hypothesis:** test candidate. + +## Current Tests Review + +No tests in the file, so R1 is skipped. + +## Coverage And Decision Analysis + +| Behavior group | Classification | Decision | +| --- | --- | --- | +| The authentication layer protects `/api/v1` routes and is built from the tokens passed in; the health check is outside it | 3A unit | R2: call `router` in-process; the health check answers `200` without a token, a v1 route answers the `unauthorized` failure without one and `200` with a valid one. The middleware's own cases are done in S1; here only the wiring (applied to v1, not to the health check, fed the given tokens). | +| Request id: generated when absent, kept when provided, copied to the response | 3A unit | R2: a request without `x-request-id` gets a response with a UUID in that header; a request with `x-request-id: my-request-1` gets the same value back. | +| Compression | 3A unit | R2: with `Accept-Encoding: gzip` a v1 response has `content-encoding: gzip`; without it, none. Only the header is asserted (decoding the body is `tower-http`'s). | +| The `408` timeout (`HandleErrorLayer` closure, line 144, plus the 5 s constant) | **Question 1** | See below. | +| Request, response and failure log events (lines 67-136) | **Question 2** | See below. | +| The server-error log arm and the failure closure (100, 102, 124-129, 136) | Coverage side effect | They run for any `5xx` response, so the unauthorized request of the first group executes them. The plan does not claim they are asserted: their content is observability, and the log text is asserted by an integration test (`tests/server/v1/contract/authentication.rs`, the `ERROR ... API ... ` line). | + +### Boundary And Arrange + +The tests call `routes::router(&container, tokens, &service_binding)` and send requests with +`tower::ServiceExt::oneshot`. The container comes from `test_support::api_container(&ephemeral())`; the tokens are a +literal `AccessTokens` (`test-token`) built in the test, not read from the configuration; the binding is +`ServiceBinding::new(Protocol::HTTP, 127.0.0.1:1212)`. No socket is opened. + +### Cases For R2 (literal expectations) + +| Request | Expected | +| --- | --- | +| `GET /api/health_check`, no token | `200` | +| `GET /api/v1/stats`, no token | `500`, `text/plain; charset=utf-8`, `Unhandled rejection: Err { reason: "unauthorized" }` | +| `GET /api/v1/stats?token=wrong` | `500`, the `token not valid` body | +| `GET /api/v1/stats?token=test-token` | `200` | +| `GET /api/health_check`, no `x-request-id` | response header `x-request-id` is a valid UUID (parsed with `uuid`) | +| `GET /api/health_check`, `x-request-id: my-request-1` | response header `x-request-id` equals `my-request-1` | +| `GET /api/v1/stats?token=test-token`, `Accept-Encoding: gzip` | `content-encoding: gzip` | +| the same without `Accept-Encoding` | no `content-encoding` header | + +## Property-Test Decision + +Not flagged and not selected: the layers are wiring with a handful of enumerable outcomes. + +## Dependencies + +None new: `uuid` (already a dev-dependency) parses the generated id, and `rstest`, `tower` and the helper exist. + +## Steps + +| ID | Status | Work | Completion evidence | +| --- | --- | --- | --- | +| R1 | SKIPPED | No existing tests. | None. | +| R2 | TODO | Add the in-file test module with the cases above. | Focused tests, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | +| R3 | TODO | Integration increment: `none` expected. | The integration tests keep the wire and log contract. | +| R4 | TODO | Re-run unit-only coverage; record before and after. | Results below and `coverage-evidence.md`. | + +## Named Mutations + +| ID | Site | Temporary change | Expected killer | +| --- | --- | --- | --- | +| L1 | Authentication layer | Add the health check route before `.layer(auth)` so the layer covers it. | the public health check case | +| L2 | Authentication layer | Remove the `.layer(middleware::from_fn_with_state(state, auth))` call. | the `unauthorized` and `token not valid` cases | +| L3 | Authentication state | Build `State` with no tokens (`AccessTokens::default()`). | the valid-token case | +| L4 | Request-id propagation | Remove `PropagateHeaderLayer`. | the provided-id case and the generated-id case | +| L5 | Compression | Remove `CompressionLayer`. | the gzip case | +| L6 | Request-id setting (first layer) | Remove the inner `SetRequestIdLayer`. | expected to **survive**: the second layer already sets the header. | +| L7 | Request-id setting (second layer) | Remove the outer `SetRequestIdLayer`. | the generated-id case | + +`router` adds `SetRequestIdLayer::x_request_id(MakeRequestUuid)` twice. If L6 survives, the inner one is +redundant; that is an observation for `lessons.md`, not a finding. + +## Maintainer Questions + +1. **The `408` timeout.** The timeout is a private 5 s constant and every production route answers quickly, so a unit + test has no way to make a request outlast it. Options: (a) record it as not testable at unit level (the + closure at line 144 and the constant stay uncovered by an assertion; the reason and owner go in the plan), + (b) a small testability refactor that lets a test pass the base routes and the timeout (`router` would call it + with the real routes and the 5 s default), which touches production code and needs your approval, or (c) a + package integration test with a deliberately slow route (it would still need the same seam). Which one? +2. **Asserting the log events.** The global capture helper of `test-helpers` installs a process-wide subscriber and + is shared with other tests; in a unit-test binary that already sets one up, a second `init()` would panic. A + scoped capture needs `tracing-subscriber` as a new dev-dependency. Options: (a) do not assert logs at unit level + (observability; the integration test asserts the `ERROR` line), (b) add `tracing-subscriber` as a dev-dependency + and assert the request, response and failure events. Which one? + +## Prose-First AAA Review + +To be written for every added test before the completed-file review. + +## Results + +Pending. From da254b3dfc261e221c25434e0ab5e97f15a980e8 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 13:27:38 +0000 Subject: [PATCH 06/17] docs(issues): [#2637] approve the router layers plan with a testability 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> --- .../ISSUE.md | 12 +++-- .../test-refactor-plans/README.md | 2 +- .../test-refactor-plans/routes-tests.md | 51 ++++++++++++------- 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index bf119aaf5..6889375c5 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -9,7 +9,7 @@ github-issue: 2637 spec-path: docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md branch: "2637-1347-add-tests-axum-rest-api-server-routes" related-pr: null -last-updated-utc: "2026-10-28 13:25" +last-updated-utc: "2026-10-28 13:27" semantic-links: skill-links: - create-issue @@ -242,7 +242,9 @@ is: ### Out of Scope - Production behavior changes in `packages/axum-rest-api-server/src/`. A bug fix or production - clean-up found here goes to a separate issue through the findings procedure. + clean-up found here goes to a separate issue through the findings procedure. One exception, approved by the + maintainer on 2026-10-28: the behavior-preserving testability refactor of `routes.rs` (plan R0), in its own + `refactor` commit. - Tracker business rules and persistence owned by `tracker-core`, use-case orchestration and port traits owned by `rest-api-application`, adapters and container wiring owned by `rest-api-runtime-adapter`, DTOs and their serialization owned by `rest-api-protocol`, the HTTP @@ -534,7 +536,7 @@ declaration-only rows have no executable entries. | ---: | --- | ---: | --- | ---: | ---: | --- | --- | --- | | 1 | `v1/middlewares/auth.rs` | 1 | S1 | 71 / 103 (68.93%) | 4 | Property candidate | DONE | [v1-middlewares-auth-tests.md](test-refactor-plans/v1-middlewares-auth-tests.md) (done; 142 / 142 lines after, 27 test cases, 8 mutants killed, property test declined) | | 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | DONE | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (done; 111 / 111 lines after, 19 test cases, 6 mutants killed, private test_support helper) | -| 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | IN_PROGRESS | [routes-tests.md](test-refactor-plans/routes-tests.md) (proposed) | +| 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | IN_PROGRESS | [routes-tests.md](test-refactor-plans/routes-tests.md) (approved 2026-10-28) | | 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | PENDING | - | | 5 | `v1/responses.rs` | 3 | S1 | 8 / 40 (20.00%) | 0 | Test candidate | DONE | [v1-responses-tests.md](test-refactor-plans/v1-responses-tests.md) (done; 66 / 66 lines after, 8 test cases, 8 mutants killed) | | 6 | `v1/context/auth_key/handlers.rs` | 4 | S3 | 0 / 56 (0.00%) | 0 | Test candidate | PENDING | - | @@ -682,6 +684,10 @@ once. If the slicing changes, the Slice column changes with it. the completed file (2026-10-28). Proposed the row 3 plan (`routes.rs`: the authentication layer wiring, request id, compression), with two questions for the maintainer (the `408` timeout has no test seam; asserting log events needs a new dev-dependency). +- 2026-10-28 13:27 UTC - Jose Celano - Approved the row 3 plan (`routes.rs`) and answered its two questions: do not + assert log events at unit level, and make the `408` timeout testable with a small behavior-preserving refactor + (R0) in this issue. This extends the scope with one exception to the production-change exclusion, recorded in + Out of Scope; the refactor is its own `refactor` commit before any test. ## Acceptance Criteria diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md index 6ee1e3052..34349c930 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md @@ -28,7 +28,7 @@ approves it. A group of declaration-only files may share one plan. | 1 | [v1-middlewares-auth-tests.md](v1-middlewares-auth-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-26 | | 2 | [v1-responses-tests.md](v1-responses-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-27 | | 3 | [v1-routes-tests.md](v1-routes-tests.md) | DONE | Approved 2026-10-28; completed file approved 2026-10-28 | -| 4 | [routes-tests.md](routes-tests.md) | PROPOSED | Awaiting maintainer approval and two answers | +| 4 | [routes-tests.md](routes-tests.md) | APPROVED | Approved 2026-10-28 with a testability refactor (R0) | Add a row when a plan is created; the remaining files and their order are in the ledger. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md index 22e9ce133..cb6dc61ca 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md @@ -3,7 +3,7 @@ doc-type: file-test-plan issue: 2637 package: torrust-tracker-axum-rest-api-server target-file: packages/axum-rest-api-server/src/routes.rs -status: proposed +status: approved semantic-links: related-artifacts: - packages/axum-rest-api-server/src/routes.rs @@ -41,14 +41,29 @@ No tests in the file, so R1 is skipped. | The authentication layer protects `/api/v1` routes and is built from the tokens passed in; the health check is outside it | 3A unit | R2: call `router` in-process; the health check answers `200` without a token, a v1 route answers the `unauthorized` failure without one and `200` with a valid one. The middleware's own cases are done in S1; here only the wiring (applied to v1, not to the health check, fed the given tokens). | | Request id: generated when absent, kept when provided, copied to the response | 3A unit | R2: a request without `x-request-id` gets a response with a UUID in that header; a request with `x-request-id: my-request-1` gets the same value back. | | Compression | 3A unit | R2: with `Accept-Encoding: gzip` a v1 response has `content-encoding: gzip`; without it, none. Only the header is asserted (decoding the body is `tower-http`'s). | -| The `408` timeout (`HandleErrorLayer` closure, line 144, plus the 5 s constant) | **Question 1** | See below. | -| Request, response and failure log events (lines 67-136) | **Question 2** | See below. | +| The `408` timeout (`HandleErrorLayer` closure, line 144, plus the 5 s constant) | 3A unit after a seam | R0 (below) splits `router` into the route composition and a private `with_layers(routes, access_tokens, binding, timeout)`. R2: pass a test route that never answers, a 50 ms timeout and a valid token; expect `408` and an empty body. The 5 s default is not asserted (see the mutation list). | +| Request, response and failure log events (lines 67-136) | Not selected at unit level | Maintainer decision 2026-10-28: do not assert log events here; no new dev-dependency. The integration test asserts the `ERROR` line with the request id. | | The server-error log arm and the failure closure (100, 102, 124-129, 136) | Coverage side effect | They run for any `5xx` response, so the unauthorized request of the first group executes them. The plan does not claim they are asserted: their content is observability, and the log text is asserted by an integration test (`tests/server/v1/contract/authentication.rs`, the `ERROR ... API ... ` line). | +### R0: The Testability Refactor + +Maintainer decision 2026-10-28 (Question 1, option b): a small, behavior-preserving refactor of `routes.rs`, done in this +issue and in its own commit before any test, so the `408` handler can be reached. + +- Add `const API_URL_PREFIX: &str = "/api";` and keep `DEFAULT_REQUEST_TIMEOUT` (5 s). +- `router` keeps its signature and its `#[instrument]`: it composes `v1::routes::add(API_URL_PREFIX, Router::new(), + container)` and calls `with_layers(routes, access_tokens, binding, DEFAULT_REQUEST_TIMEOUT)`. +- A private `with_layers(routes: Router, access_tokens: Arc, binding: &ServiceBinding, timeout: Duration) + -> Router` holds the existing chain unchanged: the authentication layer, then the public health check route, then + compression, request id, propagation, tracing and the timeout, in the same order, with `timeout` replacing the + constant. +- No other file changes. Validation that it preserves behavior: the 58 integration tests and the unit tests pass + unchanged, plus the new tests for the layers. + ### Boundary And Arrange -The tests call `routes::router(&container, tokens, &service_binding)` and send requests with -`tower::ServiceExt::oneshot`. The container comes from `test_support::api_container(&ephemeral())`; the tokens are a +The tests call `routes::router(&container, tokens, &service_binding)` (and, for the timeout only, the private +`with_layers` with a test route and a short timeout) and send requests with `tower::ServiceExt::oneshot`. The container comes from `test_support::api_container(&ephemeral())`; the tokens are a literal `AccessTokens` (`test-token`) built in the test, not read from the configuration; the binding is `ServiceBinding::new(Protocol::HTTP, 127.0.0.1:1212)`. No socket is opened. @@ -64,6 +79,7 @@ literal `AccessTokens` (`test-token`) built in the test, not read from the confi | `GET /api/health_check`, `x-request-id: my-request-1` | response header `x-request-id` equals `my-request-1` | | `GET /api/v1/stats?token=test-token`, `Accept-Encoding: gzip` | `content-encoding: gzip` | | the same without `Accept-Encoding` | no `content-encoding` header | +| `with_layers` with a route that never answers, a 50 ms timeout, `GET /api/v1/slow?token=test-token`, awaited under an absolute 5 s deadline | `408`, empty body | ## Property-Test Decision @@ -77,6 +93,7 @@ None new: `uuid` (already a dev-dependency) parses the generated id, and `rstest | ID | Status | Work | Completion evidence | | --- | --- | --- | --- | +| R0 | TODO | The behavior-preserving testability refactor above, in its own `refactor` commit. | The existing unit and integration tests pass unchanged, `cargo +nightly fmt --all -- --check`, `cargo clippy`. | | R1 | SKIPPED | No existing tests. | None. | | R2 | TODO | Add the in-file test module with the cases above. | Focused tests, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | | R3 | TODO | Integration increment: `none` expected. | The integration tests keep the wire and log contract. | @@ -93,23 +110,21 @@ None new: `uuid` (already a dev-dependency) parses the generated id, and `rstest | L5 | Compression | Remove `CompressionLayer`. | the gzip case | | L6 | Request-id setting (first layer) | Remove the inner `SetRequestIdLayer`. | expected to **survive**: the second layer already sets the header. | | L7 | Request-id setting (second layer) | Remove the outer `SetRequestIdLayer`. | the generated-id case | +| L8 | Timeout layer | Remove the `TimeoutLayer`. | the timeout case (a bounded await fails instead of hanging) | +| L9 | Timeout error mapping | Answer `INTERNAL_SERVER_ERROR` in the `HandleErrorLayer` closure. | the timeout case | +| L10 | Default timeout | Change `DEFAULT_REQUEST_TIMEOUT` to 50 s. | expected to **survive**: the default is not asserted, because that would need a 5 s wait or a tautology; recorded as a known gap. | `router` adds `SetRequestIdLayer::x_request_id(MakeRequestUuid)` twice. If L6 survives, the inner one is redundant; that is an observation for `lessons.md`, not a finding. -## Maintainer Questions - -1. **The `408` timeout.** The timeout is a private 5 s constant and every production route answers quickly, so a unit - test has no way to make a request outlast it. Options: (a) record it as not testable at unit level (the - closure at line 144 and the constant stay uncovered by an assertion; the reason and owner go in the plan), - (b) a small testability refactor that lets a test pass the base routes and the timeout (`router` would call it - with the real routes and the 5 s default), which touches production code and needs your approval, or (c) a - package integration test with a deliberately slow route (it would still need the same seam). Which one? -2. **Asserting the log events.** The global capture helper of `test-helpers` installs a process-wide subscriber and - is shared with other tests; in a unit-test binary that already sets one up, a second `init()` would panic. A - scoped capture needs `tracing-subscriber` as a new dev-dependency. Options: (a) do not assert logs at unit level - (observability; the integration test asserts the `ERROR` line), (b) add `tracing-subscriber` as a dev-dependency - and assert the request, response and failure events. Which one? +## Maintainer Decisions + +Recorded on 2026-10-28. + +1. **The `408` timeout:** a small behavior-preserving testability refactor (R0), done in this issue. The spec's + scope is extended for it (see the specification's Scope). +2. **Asserting log events:** not selected; the integration test asserts the `ERROR` line. +3. **Plan approved** with these answers. ## Prose-First AAA Review From 36242c591e69457bbbc89aecf7d4b69c3bf956a4 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 13:28:43 +0000 Subject: [PATCH 07/17] refactor(axum-rest-api-server): [#2637] separate the router layers 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> --- packages/axum-rest-api-server/src/routes.rs | 28 +++++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/packages/axum-rest-api-server/src/routes.rs b/packages/axum-rest-api-server/src/routes.rs index db4b4348b..bf8fdad2c 100644 --- a/packages/axum-rest-api-server/src/routes.rs +++ b/packages/axum-rest-api-server/src/routes.rs @@ -30,6 +30,8 @@ use tracing::{Level, Span, instrument}; const DEFAULT_REQUEST_TIMEOUT: Duration = Duration::from_secs(5); +const API_URL_PREFIX: &str = "/api"; + use super::v1; use super::v1::context::health_check::handlers::health_check_handler; use super::v1::middlewares::auth::State; @@ -41,22 +43,32 @@ pub fn router( http_api_container: &Arc, access_tokens: Arc, server_service_binding: &ServiceBinding, +) -> Router { + let routes = v1::routes::add(API_URL_PREFIX, Router::new(), http_api_container); + + with_layers(routes, access_tokens, server_service_binding, DEFAULT_REQUEST_TIMEOUT) +} + +/// Adds the authentication, the public health check and the transport layers to the API routes. +/// +/// The authentication layer is added before the health check route, so it protects the API routes and not the health +/// check. The timeout is a parameter so that a test can use a short one. +fn with_layers( + routes: Router, + access_tokens: Arc, + server_service_binding: &ServiceBinding, + timeout: Duration, ) -> Router { let server_socket_addr = server_service_binding.bind_address(); let request_service_binding = server_service_binding.clone(); let response_service_binding = server_service_binding.clone(); let failure_service_binding = server_service_binding.clone(); - let router = Router::new(); - - let api_url_prefix = "/api"; - - let router = v1::routes::add(api_url_prefix, router, http_api_container); let state = State { access_tokens }; - router + routes .layer(middleware::from_fn_with_state(state, v1::middlewares::auth::auth)) - .route(&format!("{api_url_prefix}/health_check"), get(health_check_handler)) + .route(&format!("{API_URL_PREFIX}/health_check"), get(health_check_handler)) .layer(CompressionLayer::new()) .layer(SetRequestIdLayer::x_request_id(MakeRequestUuid)) .layer(PropagateHeaderLayer::new(HeaderName::from_static("x-request-id"))) @@ -142,6 +154,6 @@ pub fn router( // this middleware goes above `TimeoutLayer` because it will receive // errors returned by `TimeoutLayer` .layer(HandleErrorLayer::new(|_: BoxError| async { StatusCode::REQUEST_TIMEOUT })) - .layer(TimeoutLayer::new(DEFAULT_REQUEST_TIMEOUT)), + .layer(TimeoutLayer::new(timeout)), ) } From 528f6b718ff2de3813a639e97decb7563a34c93e Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 16:23:12 +0000 Subject: [PATCH 08/17] test(axum-rest-api-server): [#2637] cover the router layers 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> --- .../ISSUE.md | 2 +- .../coverage-evidence.md | 4 + .../lessons.md | 1 + .../test-refactor-plans/README.md | 2 +- .../test-refactor-plans/routes-tests.md | 50 ++++- packages/axum-rest-api-server/src/routes.rs | 183 ++++++++++++++++++ 6 files changed, 233 insertions(+), 9 deletions(-) diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index 6889375c5..045c5035e 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -536,7 +536,7 @@ declaration-only rows have no executable entries. | ---: | --- | ---: | --- | ---: | ---: | --- | --- | --- | | 1 | `v1/middlewares/auth.rs` | 1 | S1 | 71 / 103 (68.93%) | 4 | Property candidate | DONE | [v1-middlewares-auth-tests.md](test-refactor-plans/v1-middlewares-auth-tests.md) (done; 142 / 142 lines after, 27 test cases, 8 mutants killed, property test declined) | | 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | DONE | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (done; 111 / 111 lines after, 19 test cases, 6 mutants killed, private test_support helper) | -| 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | IN_PROGRESS | [routes-tests.md](test-refactor-plans/routes-tests.md) (approved 2026-10-28) | +| 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | DONE | [routes-tests.md](test-refactor-plans/routes-tests.md) (done; 174 / 174 lines after, 9 test cases, 8 of 10 mutants killed, the 2 survivors recorded, R0 refactor) | | 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | PENDING | - | | 5 | `v1/responses.rs` | 3 | S1 | 8 / 40 (20.00%) | 0 | Test candidate | DONE | [v1-responses-tests.md](test-refactor-plans/v1-responses-tests.md) (done; 66 / 66 lines after, 8 test cases, 8 mutants killed) | | 6 | `v1/context/auth_key/handlers.rs` | 4 | S3 | 0 / 56 (0.00%) | 0 | Test candidate | PENDING | - | diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md index fa948d1c6..fd36c1412 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md @@ -217,3 +217,7 @@ Unit-only lines move 792 / 1,240 (63.87%) at the S1 baseline to 921 / 1,305 (70. ### `v1/routes.rs` (slice S2) Unit-only (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): lines 27 / 45 (60.00%) before, 111 / 111 (100.00%) after; regions 51 / 73 before, 174 / 174 after; functions 1 / 3 before, 13 / 13 after. The counts include the in-file test module. The new crate-private `test_support.rs` helper is 31 / 31 lines. Nineteen test cases; six named mutations killed (see [the plan](test-refactor-plans/v1-routes-tests.md)). + +### `routes.rs` (slice S2) + +Unit-only (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): lines 58 / 67 (86.57%) before, 174 / 174 (100.00%) after; regions 103 / 117 before, 280 / 280 after; functions 5 / 8 before, 26 / 26 after. The counts include the in-file test module. The request, response and failure log closures run but their content is not asserted at unit level (an integration test asserts the `ERROR` line). Nine test cases; eight of ten named mutations killed, two survive by design (see [the plan](test-refactor-plans/routes-tests.md)). diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md index 107f7723d..e34c02324 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/lessons.md @@ -19,6 +19,7 @@ Candidate lessons and follow-ups collected while working on Issue #2637. Promoti | [Routes plan](test-refactor-plans/v1-routes-tests.md) | Design question: make the state "private or listed set but no persistence" unrepresentable. `TrackerCoreContainer` holds `core_config.private` and `persistence: Option` as independent fields, and `v1/routes.rs` re-derives service availability from them, which ADR `20260825193119` (agreement 2) says route handlers should not duplicate. Composing the capabilities once (for example `Option` handles built where the database check happens) would remove the combination. It touches `tracker-core` and `rest-api-runtime-adapter`, so it is outside #2637. Maintainer decision 2026-10-28: record here only for now. | The first line of defense is `validate_persistence_requirements` in `src/bootstrap/persistence.rs`, called by `setup` in `src/bootstrap/app.rs`; the second is `v1/routes.rs`, protected by the plan's no-persistence cases. | | `src/bootstrap/persistence.rs` module comment | Fix the stale comment: it says the check is "intentionally not called" while the runtime uses v2 configuration, but `setup` in `src/bootstrap/app.rs` calls it. Not a behavior bug and outside the package under test. | `grep -n validate_persistence_requirements src/bootstrap/app.rs` finds the call. | | [Routes plan](test-refactor-plans/v1-routes-tests.md) | Clarify the test-support file conventions in ADR `20261021190318` in a separate small PR after #2637: (1) where a crate-private helper (rule C) lives and how it is marked: a private `#[cfg(test)] mod` with an inner `#![cfg(test)]` in its file, versus a `#[cfg(test)]` submodule under `src/testing/`, and whether `src/testing/` holds only shared, feature-gated code (rule A) or any test code outside a `#[cfg(test)] mod tests`; (2) the naming convention, noted by the maintainer on 2026-10-28, that a file containing only test code is prefixed `test_` (as `test_support.rs`, and `tracker-core`'s `test_helpers.rs`); (3) whether the `tracker-core` crate-root helpers follow it. | The ADR says nothing about the folder of a rule C helper. `git log` shows the moves of `src/environment.rs` into `src/testing/environment.rs` (#1903 and siblings) "marking it explicitly as test-only infrastructure", all of them shared environments. `tracker-core` keeps `test_helpers.rs` and `whitelist/test_helpers.rs` at its crate root with `#[cfg(test)]` content. | +| [Router layers plan](test-refactor-plans/routes-tests.md) | `router` adds `SetRequestIdLayer::x_request_id(MakeRequestUuid)` twice. Removing the inner one changes nothing a client sees (mutation L6 survived); decide whether to drop it. Also the default 5 s timeout is not asserted (mutation L10 survived). Neither is a bug. | `packages/axum-rest-api-server/src/routes.rs`; the plan's Results. | ## Testing Lessons diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md index 34349c930..6ec0a34f7 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md @@ -28,7 +28,7 @@ approves it. A group of declaration-only files may share one plan. | 1 | [v1-middlewares-auth-tests.md](v1-middlewares-auth-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-26 | | 2 | [v1-responses-tests.md](v1-responses-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-27 | | 3 | [v1-routes-tests.md](v1-routes-tests.md) | DONE | Approved 2026-10-28; completed file approved 2026-10-28 | -| 4 | [routes-tests.md](routes-tests.md) | APPROVED | Approved 2026-10-28 with a testability refactor (R0) | +| 4 | [routes-tests.md](routes-tests.md) | DONE | Approved 2026-10-28 with a testability refactor (R0); completed file approved 2026-10-28 | Add a row when a plan is created; the remaining files and their order are in the ledger. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md index cb6dc61ca..76fe3dcdc 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md @@ -3,7 +3,7 @@ doc-type: file-test-plan issue: 2637 package: torrust-tracker-axum-rest-api-server target-file: packages/axum-rest-api-server/src/routes.rs -status: approved +status: done semantic-links: related-artifacts: - packages/axum-rest-api-server/src/routes.rs @@ -93,11 +93,11 @@ None new: `uuid` (already a dev-dependency) parses the generated id, and `rstest | ID | Status | Work | Completion evidence | | --- | --- | --- | --- | -| R0 | TODO | The behavior-preserving testability refactor above, in its own `refactor` commit. | The existing unit and integration tests pass unchanged, `cargo +nightly fmt --all -- --check`, `cargo clippy`. | +| R0 | DONE | The behavior-preserving testability refactor above, in its own `refactor` commit. | The existing unit and integration tests pass unchanged, `cargo +nightly fmt --all -- --check`, `cargo clippy`. | | R1 | SKIPPED | No existing tests. | None. | -| R2 | TODO | Add the in-file test module with the cases above. | Focused tests, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | -| R3 | TODO | Integration increment: `none` expected. | The integration tests keep the wire and log contract. | -| R4 | TODO | Re-run unit-only coverage; record before and after. | Results below and `coverage-evidence.md`. | +| R2 | DONE | Add the in-file test module with the cases above. | Focused tests, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | +| R3 | DONE | Integration increment: `none` expected. | The integration tests keep the wire and log contract. | +| R4 | DONE | Re-run unit-only coverage; record before and after. | Results below and `coverage-evidence.md`. | ## Named Mutations @@ -128,8 +128,44 @@ Recorded on 2026-10-28. ## Prose-First AAA Review -To be written for every added test before the completed-file review. +Temporary prose, written before comparing with the code. + +| Test (cases) | Arrange prose | Act prose | Assert prose | Outcome | +| --- | --- | --- | --- | --- | +| `it_should_serve_the_health_check_without_a_token` | A request to the health check with no token. | Send it through the router. | `200`. | Only the status: the body belongs to the health check handler (row 4). Kept. | +| `it_should_reject_a_request_to_an_api_route_without_a_configured_token` (2) | A v1 request with no token, or with a wrong one. | Same. | `500` and the literal `unauthorized` or `token not valid` body. | The middleware's own cases were done in S1; this pins that the router applies it to v1 and feeds it the tokens. Kept. | +| `it_should_serve_an_api_route_when_the_request_carries_a_token_given_to_the_router` | A v1 request with the literal token that the test gave the router. | Same. | `200`. | The token is a literal in the test, not read from the configuration. Kept. | +| `it_should_generate_a_request_id_when_the_request_has_none` | A request with no `x-request-id`. | Same. | The response header parses as a UUID. | The value is random, so the property is the format. Kept. | +| `it_should_return_the_request_id_provided_by_the_client` | A request with `x-request-id: my-request-1`. | Same. | The response carries exactly that value. | Kept. | +| `it_should_compress_the_response_only_as_the_client_asks` (2) | A v1 request with `Accept-Encoding: gzip`, or without it. | Same. | `content-encoding: gzip`, or none. | Only the header: decoding is `tower-http`'s. Kept. | +| `it_should_answer_request_timeout_when_an_api_route_does_not_answer_within_the_timeout` | A route that never answers, a 50 ms timeout, a token. | `with_layers` through `oneshot`, awaited under an absolute 5 s deadline. | `408` and an empty body. | The deadline makes a missing timeout fail instead of hang (L8 fails after 5 s). Kept. | + +Helpers: `received` reads the status, headers and body; `through_the_router` builds the container and the router for a +default tracker; `access_tokens` and `service_binding` hide only construction. The expected values are literals. ## Results -Pending. +- **Unit-only coverage** (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): `routes.rs` 58 / 67 lines + (86.57%) before; 174 / 174 lines (100.00%), 280 / 280 regions and 26 / 26 functions after (the counts include + the test module). Package unit-only lines after this file: 1,258 / 1,509 (83.37%), regions 1,838 / 2,305 + (79.74%), functions 178 / 226 (78.76%), 74 unit tests. +- **Honest note on coverage:** the request, response and failure log closures (lines 67-136) are now executed, because + the unauthorized request goes through the router and a `5xx` response runs the server-error and failure arms. No + test asserts their content (maintainer decision: no unit-level log assertions). The `ERROR ... API ... ` + line is asserted by an integration test, so those lines are covered, not protected, at unit level. +- **Tests:** 9 test cases in seven tests (two `rstest` tables of 2). No integration test was added or changed. +- **R0 refactor:** commit `refactor(axum-rest-api-server): [#2637] separate the router layers from the routes`. A pure + move: the chain and its order are unchanged, `timeout` replaces the constant, and the prefix is a constant. The + 58 integration tests and the 65 unit tests of the moment passed unchanged before the tests were added. +- **Mutations** (each applied alone, stable toolchain, then reverted; the file was identical to the tested one + afterwards): L1 killed by the public health check test, L2 by both rejection cases, L3 by the valid-token and the + timeout tests, L4 by both request-id tests, L5 by the gzip case, L7 by the generated-id test, L8 by the timeout test + (after its 5 s deadline) and L9 by the timeout test. **L6 survived, as predicted:** removing the inner + `SetRequestIdLayer` changes nothing a client can see, because the outer one already sets the header, so `router` + carries a redundant layer. **L10 survived:** the 5 s default is not asserted, because that would need a 5 s wait + or a tautology. Both are recorded in `lessons.md`; neither is a finding. L8 and L9 were run again after the + timeout assertion was simplified to status and body. +- **Validation:** `cargo +nightly fmt --all -- --check` (nightly Rust toolchain), `cargo clippy -p + torrust-tracker-axum-rest-api-server --all-targets` (stable), `cargo machete --with-metadata`, `git diff + --check` and `cargo test -p torrust-tracker-axum-rest-api-server` (74 unit and 58 integration tests) pass. +- **Review outcome:** the maintainer approved the completed file on 2026-10-28, including the R0 refactor. diff --git a/packages/axum-rest-api-server/src/routes.rs b/packages/axum-rest-api-server/src/routes.rs index bf8fdad2c..7078b2ef0 100644 --- a/packages/axum-rest-api-server/src/routes.rs +++ b/packages/axum-rest-api-server/src/routes.rs @@ -157,3 +157,186 @@ fn with_layers( .layer(TimeoutLayer::new(timeout)), ) } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + use std::time::Duration; + + use axum::Router; + use axum::body::{Body, to_bytes}; + use axum::http::header::{ACCEPT_ENCODING, CONTENT_ENCODING}; + use axum::http::{HeaderMap, Request, StatusCode}; + use axum::routing::get; + use rstest::rstest; + use secrecy::SecretString; + use torrust_net_primitives::service_binding::{Protocol, ServiceBinding}; + use torrust_tracker_configuration::v3_0_0::tracker_api::AccessTokens; + use torrust_tracker_test_helpers::configuration::ephemeral; + use tower::ServiceExt; + use uuid::Uuid; + + use super::{router, with_layers}; + use crate::test_support::api_container; + + const TOKEN: &str = "test-token"; + const DEADLINE: Duration = Duration::from_secs(5); + + /// What a client receives: the status, the headers and the body. + struct Received { + status: StatusCode, + headers: HeaderMap, + body: String, + } + + fn access_tokens() -> Arc { + Arc::new(AccessTokens::from([("test".to_string(), SecretString::from(TOKEN))])) + } + + fn service_binding() -> ServiceBinding { + ServiceBinding::new(Protocol::HTTP, "127.0.0.1:1212".parse().expect("the address is valid")) + .expect("the service binding is valid") + } + + fn get_request(uri: &str) -> Request { + Request::builder() + .uri(uri) + .body(Body::empty()) + .expect("the request is well formed") + } + + async fn received(response: axum::response::Response) -> Received { + let status = response.status(); + let headers = response.headers().clone(); + let body = to_bytes(response.into_body(), usize::MAX) + .await + .expect("the body is readable"); + + Received { + status, + headers, + body: String::from_utf8_lossy(&body).into_owned(), + } + } + + /// Sends the request through the API router built for a default tracker. + async fn through_the_router(request: Request) -> Received { + let container = api_container(&ephemeral()).await; + let router = router(&container, access_tokens(), &service_binding()); + + received(router.oneshot(request).await.expect("the router never fails")).await + } + + #[tokio::test] + async fn it_should_serve_the_health_check_without_a_token() { + let request = get_request("/api/health_check"); + + let received = through_the_router(request).await; + + assert_eq!(received.status, StatusCode::OK); + } + + #[rstest] + #[case::no_token("/api/v1/stats", r#"Unhandled rejection: Err { reason: "unauthorized" }"#)] + #[case::a_wrong_token( + "/api/v1/stats?token=wrong-token", + r#"Unhandled rejection: Err { reason: "token not valid" }"# + )] + #[tokio::test] + async fn it_should_reject_a_request_to_an_api_route_without_a_configured_token(#[case] uri: &str, #[case] body: &str) { + let request = get_request(uri); + + let received = through_the_router(request).await; + + assert_eq!( + (received.status, received.body), + (StatusCode::INTERNAL_SERVER_ERROR, body.to_string()) + ); + } + + #[tokio::test] + async fn it_should_serve_an_api_route_when_the_request_carries_a_token_given_to_the_router() { + let request = get_request("/api/v1/stats?token=test-token"); + + let received = through_the_router(request).await; + + assert_eq!(received.status, StatusCode::OK); + } + + #[tokio::test] + async fn it_should_generate_a_request_id_when_the_request_has_none() { + let request = get_request("/api/health_check"); + + let received = through_the_router(request).await; + + let request_id = received + .headers + .get("x-request-id") + .expect("the response has a request id") + .to_str() + .expect("the request id is text"); + assert!( + Uuid::parse_str(request_id).is_ok(), + "the request id is not a UUID: {request_id}" + ); + } + + #[tokio::test] + async fn it_should_return_the_request_id_provided_by_the_client() { + let request = Request::builder() + .uri("/api/health_check") + .header("x-request-id", "my-request-1") + .body(Body::empty()) + .expect("the request is well formed"); + + let received = through_the_router(request).await; + + assert_eq!( + received.headers.get("x-request-id").map(axum::http::HeaderValue::as_bytes), + Some(&b"my-request-1"[..]) + ); + } + + #[rstest] + #[case::when_the_client_accepts_gzip(Some("gzip"), Some("gzip"))] + #[case::when_the_client_does_not_ask_for_compression(None, None)] + #[tokio::test] + async fn it_should_compress_the_response_only_as_the_client_asks( + #[case] accept_encoding: Option<&str>, + #[case] content_encoding: Option<&str>, + ) { + let mut request = get_request("/api/v1/stats?token=test-token"); + if let Some(accept_encoding) = accept_encoding { + request + .headers_mut() + .insert(ACCEPT_ENCODING, accept_encoding.parse().expect("the header value is valid")); + } + + let received = through_the_router(request).await; + + assert_eq!( + received.headers.get(CONTENT_ENCODING).map(axum::http::HeaderValue::as_bytes), + content_encoding.map(str::as_bytes) + ); + } + + #[tokio::test] + async fn it_should_answer_request_timeout_when_an_api_route_does_not_answer_within_the_timeout() { + let never_answering = Router::new().route("/api/v1/slow", get(std::future::pending::<&'static str>)); + let router = with_layers( + never_answering, + access_tokens(), + &service_binding(), + Duration::from_millis(50), + ); + let request = get_request("/api/v1/slow?token=test-token"); + + let response = tokio::time::timeout(DEADLINE, router.oneshot(request)) + .await + .expect("the router answers within the deadline") + .expect("the router never fails"); + + let received = received(response).await; + assert_eq!((received.status, received.body), (StatusCode::REQUEST_TIMEOUT, String::new())); + } +} From eb0f1a99d69225da52f0d549541719321bdb71a0 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 16:24:12 +0000 Subject: [PATCH 09/17] docs(issues): [#2637] add the health check handler test plan 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> --- .../ISSUE.md | 8 +- .../test-refactor-plans/README.md | 1 + .../v1-context-health-check-handlers-tests.md | 78 +++++++++++++++++++ 3 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index 045c5035e..f2ac0417d 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -9,7 +9,7 @@ github-issue: 2637 spec-path: docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md branch: "2637-1347-add-tests-axum-rest-api-server-routes" related-pr: null -last-updated-utc: "2026-10-28 13:27" +last-updated-utc: "2026-10-28 16:24" semantic-links: skill-links: - create-issue @@ -537,7 +537,7 @@ declaration-only rows have no executable entries. | 1 | `v1/middlewares/auth.rs` | 1 | S1 | 71 / 103 (68.93%) | 4 | Property candidate | DONE | [v1-middlewares-auth-tests.md](test-refactor-plans/v1-middlewares-auth-tests.md) (done; 142 / 142 lines after, 27 test cases, 8 mutants killed, property test declined) | | 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | DONE | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (done; 111 / 111 lines after, 19 test cases, 6 mutants killed, private test_support helper) | | 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | DONE | [routes-tests.md](test-refactor-plans/routes-tests.md) (done; 174 / 174 lines after, 9 test cases, 8 of 10 mutants killed, the 2 survivors recorded, R0 refactor) | -| 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | PENDING | - | +| 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | IN_PROGRESS | [v1-context-health-check-handlers-tests.md](test-refactor-plans/v1-context-health-check-handlers-tests.md) (proposed) | | 5 | `v1/responses.rs` | 3 | S1 | 8 / 40 (20.00%) | 0 | Test candidate | DONE | [v1-responses-tests.md](test-refactor-plans/v1-responses-tests.md) (done; 66 / 66 lines after, 8 test cases, 8 mutants killed) | | 6 | `v1/context/auth_key/handlers.rs` | 4 | S3 | 0 / 56 (0.00%) | 0 | Test candidate | PENDING | - | | 7 | `v1/context/auth_key/responses.rs` | 4 | S3 | 0 / 26 (0.00%) | 0 | Test candidate | PENDING | - | @@ -688,6 +688,10 @@ once. If the slicing changes, the Slice column changes with it. assert log events at unit level, and make the `408` timeout testable with a small behavior-preserving refactor (R0) in this issue. This extends the scope with one exception to the production-change exclusion, recorded in Out of Scope; the refactor is its own `refactor` commit before any test. +- 2026-10-28 16:24 UTC - GitHub Copilot - S2 row 3 (`routes.rs`) is done: unit-only lines 58 / 67 (86.57%) to 174 / 174 + (100.00%), 9 test cases, eight of ten named mutations killed (two survive by design and are recorded), and the + behavior-preserving `with_layers` refactor in its own commit; the maintainer approved the completed file (16:22 UTC). + Proposed the plan for row 4 (`v1/context/health_check/handlers.rs`), the last file of S2. ## Acceptance Criteria diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md index 6ec0a34f7..75a188a5c 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md @@ -29,6 +29,7 @@ approves it. A group of declaration-only files may share one plan. | 2 | [v1-responses-tests.md](v1-responses-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-27 | | 3 | [v1-routes-tests.md](v1-routes-tests.md) | DONE | Approved 2026-10-28; completed file approved 2026-10-28 | | 4 | [routes-tests.md](routes-tests.md) | DONE | Approved 2026-10-28 with a testability refactor (R0); completed file approved 2026-10-28 | +| 5 | [v1-context-health-check-handlers-tests.md](v1-context-health-check-handlers-tests.md) | PROPOSED | Awaiting maintainer approval | Add a row when a plan is created; the remaining files and their order are in the ledger. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md new file mode 100644 index 000000000..fa6bd6a21 --- /dev/null +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md @@ -0,0 +1,78 @@ +--- +doc-type: file-test-plan +issue: 2637 +package: torrust-tracker-axum-rest-api-server +target-file: packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs +status: proposed +semantic-links: + related-artifacts: + - packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs + - docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md + - docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md +--- + +# Health Check Handler File Test Plan + +Follow the shared [guidance](README.md). This plan covers only +`packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs`, ledger row 4 (rank 2), the last file of S2. + +## Current State + +- **Unit-only coverage:** 4 / 4 lines (100.00%), 4 / 4 regions, 2 / 2 functions. The lines run through the + test-environment unit tests and the router tests of row 3, but **no test asserts the handler's answer**: the router + tests assert only that the health check answers `200` without a token. +- **Existing tests:** none in the file. An integration test asserts the health check over HTTP. +- **Module-owned decision:** `health_check_handler` answers with a JSON `Report` whose status is `Ok`. The type and + its serialization belong to `rest-api-protocol`; the package owns that the handler reports `Ok` and wraps it as + JSON. The module documentation shows the response `{"status": "Ok"}`. +- **T1 hypothesis:** test candidate (small). + +## Current Tests Review + +No tests in the file, so R1 is skipped. + +## Coverage And Decision Analysis + +| Behavior group | Classification | Decision | +| --- | --- | --- | +| The handler answers `200`, `application/json`, body `{"status":"Ok"}` | 3A unit | R2: call `health_check_handler().await`, convert it into a response, and assert the three parts as literals. | +| `Status::Error` | Not reachable | The handler never returns it, so there is no behavior to test; the variant belongs to the DTO. | + +Coverage is already 100%, so this test adds protection, not lines. The plan says so to avoid claiming a coverage gain. + +### Case For R2 + +| Call | Expected | +| --- | --- | +| `health_check_handler().await.into_response()` | `200`, `content-type: application/json`, body `{"status":"Ok"}` | + +## Property-Test Decision + +Not flagged and not selected: a constant answer. + +## Dependencies + +None. + +## Steps + +| ID | Status | Work | Completion evidence | +| --- | --- | --- | --- | +| R1 | SKIPPED | No existing tests. | None. | +| R2 | TODO | Add the in-file test module with the one case. | Focused test, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | +| R3 | TODO | Integration increment: `none`. | The integration test keeps the wire contract. | +| R4 | TODO | Re-run unit-only coverage; record before and after. | Results below and `coverage-evidence.md`. | + +## Named Mutations + +| ID | Site | Temporary change | Expected killer | +| --- | --- | --- | --- | +| H1 | `health_check_handler` | Report `Status::Error` instead of `Status::Ok`. | the one case (body) | + +## Prose-First AAA Review + +To be written for the added test before the completed-file review. + +## Results + +Pending. From 4acf47c87707ae65a7328676502540263c0d2653 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 16:37:18 +0000 Subject: [PATCH 10/17] test(axum-rest-api-server): [#2637] cover the health check handler 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> --- .../ISSUE.md | 2 +- .../coverage-evidence.md | 4 +++ .../test-refactor-plans/README.md | 2 +- .../v1-context-health-check-handlers-tests.md | 25 ++++++++++++++----- .../src/v1/context/health_check/handlers.rs | 21 ++++++++++++++++ 5 files changed, 46 insertions(+), 8 deletions(-) diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index f2ac0417d..f71078aea 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -537,7 +537,7 @@ declaration-only rows have no executable entries. | 1 | `v1/middlewares/auth.rs` | 1 | S1 | 71 / 103 (68.93%) | 4 | Property candidate | DONE | [v1-middlewares-auth-tests.md](test-refactor-plans/v1-middlewares-auth-tests.md) (done; 142 / 142 lines after, 27 test cases, 8 mutants killed, property test declined) | | 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | DONE | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (done; 111 / 111 lines after, 19 test cases, 6 mutants killed, private test_support helper) | | 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | DONE | [routes-tests.md](test-refactor-plans/routes-tests.md) (done; 174 / 174 lines after, 9 test cases, 8 of 10 mutants killed, the 2 survivors recorded, R0 refactor) | -| 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | IN_PROGRESS | [v1-context-health-check-handlers-tests.md](test-refactor-plans/v1-context-health-check-handlers-tests.md) (proposed) | +| 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | DONE | [v1-context-health-check-handlers-tests.md](test-refactor-plans/v1-context-health-check-handlers-tests.md) (done; 16 / 16 lines after, 1 test, mutant killed; protection not coverage) | | 5 | `v1/responses.rs` | 3 | S1 | 8 / 40 (20.00%) | 0 | Test candidate | DONE | [v1-responses-tests.md](test-refactor-plans/v1-responses-tests.md) (done; 66 / 66 lines after, 8 test cases, 8 mutants killed) | | 6 | `v1/context/auth_key/handlers.rs` | 4 | S3 | 0 / 56 (0.00%) | 0 | Test candidate | PENDING | - | | 7 | `v1/context/auth_key/responses.rs` | 4 | S3 | 0 / 26 (0.00%) | 0 | Test candidate | PENDING | - | diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md index fd36c1412..7e8145dc7 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md @@ -221,3 +221,7 @@ Unit-only (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): lines ### `routes.rs` (slice S2) Unit-only (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): lines 58 / 67 (86.57%) before, 174 / 174 (100.00%) after; regions 103 / 117 before, 280 / 280 after; functions 5 / 8 before, 26 / 26 after. The counts include the in-file test module. The request, response and failure log closures run but their content is not asserted at unit level (an integration test asserts the `ERROR` line). Nine test cases; eight of ten named mutations killed, two survive by design (see [the plan](test-refactor-plans/routes-tests.md)). + +### `v1/context/health_check/handlers.rs` (slice S2) + +Unit-only (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): lines 4 / 4 (100.00%) before, 16 / 16 (100.00%) after; regions 4 / 4 before, 25 / 25 after; functions 2 / 2 before, 4 / 4 after. The file was already fully covered; the single test adds protection (nothing asserted the answer), and the counts grow only by the test module. The mutation (report `Status::Error`) is killed (see [the plan](test-refactor-plans/v1-context-health-check-handlers-tests.md)). diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md index 75a188a5c..271a87459 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/README.md @@ -29,7 +29,7 @@ approves it. A group of declaration-only files may share one plan. | 2 | [v1-responses-tests.md](v1-responses-tests.md) | DONE | Approved 2026-10-26; completed file approved 2026-10-27 | | 3 | [v1-routes-tests.md](v1-routes-tests.md) | DONE | Approved 2026-10-28; completed file approved 2026-10-28 | | 4 | [routes-tests.md](routes-tests.md) | DONE | Approved 2026-10-28 with a testability refactor (R0); completed file approved 2026-10-28 | -| 5 | [v1-context-health-check-handlers-tests.md](v1-context-health-check-handlers-tests.md) | PROPOSED | Awaiting maintainer approval | +| 5 | [v1-context-health-check-handlers-tests.md](v1-context-health-check-handlers-tests.md) | DONE | Approved 2026-10-28; completed file approved 2026-10-28 | Add a row when a plan is created; the remaining files and their order are in the ledger. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md index fa6bd6a21..18b639f06 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md @@ -3,7 +3,7 @@ doc-type: file-test-plan issue: 2637 package: torrust-tracker-axum-rest-api-server target-file: packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs -status: proposed +status: done semantic-links: related-artifacts: - packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs @@ -59,9 +59,9 @@ None. | ID | Status | Work | Completion evidence | | --- | --- | --- | --- | | R1 | SKIPPED | No existing tests. | None. | -| R2 | TODO | Add the in-file test module with the one case. | Focused test, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | -| R3 | TODO | Integration increment: `none`. | The integration test keeps the wire contract. | -| R4 | TODO | Re-run unit-only coverage; record before and after. | Results below and `coverage-evidence.md`. | +| R2 | DONE | Add the in-file test module with the one case. | Focused test, `cargo +nightly fmt --all -- --check`, `cargo clippy`, `git diff --check`, prose-first AAA review, and the named mutations. | +| R3 | DONE | Integration increment: `none`. | The integration test keeps the wire contract. | +| R4 | DONE | Re-run unit-only coverage; record before and after. | Results below and `coverage-evidence.md`. | ## Named Mutations @@ -71,8 +71,21 @@ None. ## Prose-First AAA Review -To be written for the added test before the completed-file review. +Temporary prose, written before comparing with the code. + +| Test | Arrange prose | Act prose | Assert prose | Outcome | +| --- | --- | --- | --- | --- | +| `it_should_answer_ok_with_a_json_report_whose_status_is_ok` | Nothing: the handler takes no input. | Call `health_check_handler` and turn its answer into a response. | The status, the `content-type` and the body together equal `200`, `application/json` and `{"status":"Ok"}`. | The expected whole value is a literal in the test, not produced by the DTO's serializer. No helper was needed. Kept. | ## Results -Pending. +- **Unit-only coverage** (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): 4 / 4 lines (100.00%) before; 16 / 16 lines + (100.00%), 25 / 25 regions and 4 / 4 functions after. The file was already fully covered, so the line gain is only + the test module: this test adds protection, not production-line coverage. Package unit-only lines after this file: + 1,265 / 1,516 (83.44%), regions 1,856 / 2,323 (79.90%), functions 180 / 228 (78.95%), 75 unit tests. +- **Mutation H1** (report `Status::Error`) applied alone and reverted exactly: killed by the one test. +- **Tests:** one test. No integration test was added or changed. +- **Validation:** `cargo +nightly fmt --all -- --check` (nightly Rust toolchain), `cargo clippy -p + torrust-tracker-axum-rest-api-server --all-targets` (stable), `cargo machete --with-metadata`, `git diff --check` + and `cargo test -p torrust-tracker-axum-rest-api-server` (75 unit and 58 integration tests) pass. +- **Review outcome:** the maintainer approved the completed file on 2026-10-28. diff --git a/packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs b/packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs index c7851d996..a48ec2180 100644 --- a/packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs +++ b/packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs @@ -8,3 +8,24 @@ use torrust_tracker_rest_api_protocol::v1::context::health_check::resources::rep pub async fn health_check_handler() -> Json { Json(Report { status: Status::Ok }) } + +#[cfg(test)] +mod tests { + use axum::body::to_bytes; + use axum::http::StatusCode; + use axum::http::header::CONTENT_TYPE; + use axum::response::IntoResponse; + + use super::health_check_handler; + + #[tokio::test] + async fn it_should_answer_ok_with_a_json_report_whose_status_is_ok() { + let (parts, body) = health_check_handler().await.into_response().into_parts(); + + let body = to_bytes(body, usize::MAX).await.expect("the body is readable"); + assert_eq!( + (parts.status, parts.headers[CONTENT_TYPE].as_bytes(), &body[..]), + (StatusCode::OK, &b"application/json"[..], &br#"{"status":"Ok"}"#[..]) + ); + } +} From fbcd2fb897136ba12717aed0e935041cc5fab793 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 16:40:47 +0000 Subject: [PATCH 11/17] docs(issues): [#2637] record the S2 coverage measurement and update the EPIC tables Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../open/1347-overhaul-packages-testing/EPIC.md | 12 ++++++++---- .../ISSUE.md | 12 ++++++++++-- .../coverage-evidence.md | 14 +++++++++++++- .../test-refactor-plans/routes-tests.md | 7 ++++--- 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/docs/issues/open/1347-overhaul-packages-testing/EPIC.md b/docs/issues/open/1347-overhaul-packages-testing/EPIC.md index baabc5251..d312c1bc1 100644 --- a/docs/issues/open/1347-overhaul-packages-testing/EPIC.md +++ b/docs/issues/open/1347-overhaul-packages-testing/EPIC.md @@ -6,7 +6,7 @@ epic: null github-issue: 1347 spec-path: docs/issues/open/1347-overhaul-packages-testing/EPIC.md epic-owner: josecelano -last-updated-utc: "2026-10-27 14:58" +last-updated-utc: "2026-10-28 16:39" semantic-links: skill-links: - create-issue @@ -95,7 +95,7 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. | 7 | #2283 - Increase UDP server package coverage | `docs/issues/closed/2283-1347-increase-udp-server-package-coverage/ISSUE.md` | DONE | Every package source file processed through a reviewed per-file plan; unit-only lines 96.18% → 98.18%; bounded mutation sample and manual verification recorded; merged via PR #2381. | | 8 | #2301 - Review package coverage rollout | `docs/issues/open/2301-1347-review-package-coverage-rollout/ISSUE.md` | TODO | After the EPIC completes, review real rollout evidence before retaining, revising, or proposing a required coverage check. | | 9 | #2361 - Keep the report-only package coverage summary from failing on a discovery failure | `docs/issues/open/2361-1347-package-coverage-summary-discovery-failure/ISSUE.md` | TODO | Bug follow-up of #2347 (`review-finding:pr-2293-f1`): the #2222 summary check goes red with a JSON parse error when discovery fails. | -| 10 | #2637 - Add tests to the axum-rest-api-server package | `docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md` | IN_PROGRESS | S0 (spec and T1 baseline) merged through #2638; S1 (authentication and shared responses) merged through #2667; S2 (routes) started; S3-S6 follow. Replaces the former draft that was misfiled under #1349. | +| 10 | #2637 - Add tests to the axum-rest-api-server package | `docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md` | IN_PROGRESS | S0 (spec and T1 baseline) merged through #2638; S1 (authentication and shared responses) merged through #2667; S2 (routes) is implemented and awaits its pull request; S3-S6 follow. Replaces the former draft that was misfiled under #1349. | | 11 | Additional package-testing subissues | Create a folder-style spec when a concrete package need is identified | TODO | Permitted but not required upfront; retain scope in this EPIC. | ## Package Coverage Tracking @@ -119,7 +119,7 @@ level or determine whether unit coverage is adequate. | `torrust-tracker-udp-server` | [#2283](../../closed/2283-1347-increase-udp-server-package-coverage/ISSUE.md) | Lines: 97.86%; regions: 96.64%; functions: 97.64% | Lines: 98.58%; regions: 97.62%; functions: 98.56% | Lines: +0.72 pp; regions: +0.98 pp; functions: +0.92 pp | [Coverage evidence](../../closed/2283-1347-increase-udp-server-package-coverage/coverage-evidence.md) | | `torrust-tracker-udp-core` | [#1348](../../closed/1348-1347-add-tests-udp-core/ISSUE.md) | Lines: 72.49%; regions: 78.56%; functions: 71.27% | Lines: 98.04%; regions: 98.15%; functions: 97.61% | Lines: +25.55 pp; regions: +19.58 pp; functions: +26.34 pp | [Coverage evidence](../../closed/1348-1347-add-tests-udp-core/coverage-evidence.md) | | `torrust-tracker-http-core` | [#1349](../../closed/1349-1347-add-tests-http-core/ISSUE.md) | Lines: 88.88%; regions: 90.41%; functions: 84.29% | Lines: 99.28%; regions: 99.25%; functions: 99.74% | Lines: +10.40 pp; regions: +8.84 pp; functions: +15.45 pp | [Coverage evidence](../../closed/1349-1347-add-tests-http-core/coverage-evidence.md) | -| `torrust-tracker-axum-rest-api-server` | [#2637](../2637-1347-add-tests-axum-rest-api-server/ISSUE.md) | Lines: 87.98%; regions: 84.10%; functions: 83.42% | Lines: 88.97%; regions: 85.24%; functions: 85.20% | Lines: +0.99 pp; regions: +1.14 pp; functions: +1.78 pp | [Coverage evidence](../2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md) | +| `torrust-tracker-axum-rest-api-server` | [#2637](../2637-1347-add-tests-axum-rest-api-server/ISSUE.md) | Lines: 87.98%; regions: 84.10%; functions: 83.42% | Lines: 91.32%; regions: 88.23%; functions: 89.13% | Lines: +3.34 pp; regions: +4.13 pp; functions: +5.71 pp | [Coverage evidence](../2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md) | ### Unit-Only Coverage @@ -133,7 +133,7 @@ weak unit-only result with aggregate, integration, example, or end-to-end covera | `torrust-tracker-udp-server` | [#2283](../../closed/2283-1347-increase-udp-server-package-coverage/ISSUE.md) | Lines: 96.18%; regions: 95.34%; functions: 94.93% | Lines: 98.18%; regions: 97.04%; functions: 97.70% | Lines: +2.00 pp; regions: +1.70 pp; functions: +2.77 pp | [Coverage evidence](../../closed/2283-1347-increase-udp-server-package-coverage/coverage-evidence.md) | | `torrust-tracker-udp-core` | [#1348](../../closed/1348-1347-add-tests-udp-core/ISSUE.md) | Lines: 72.49%; regions: 78.56%; functions: 71.27% | Lines: 98.04%; regions: 98.15%; functions: 97.61% | Lines: +25.55 pp; regions: +19.58 pp; functions: +26.34 pp | [Coverage evidence](../../closed/1348-1347-add-tests-udp-core/coverage-evidence.md) | | `torrust-tracker-http-core` | [#1349](../../closed/1349-1347-add-tests-http-core/ISSUE.md) | Lines: 88.88%; regions: 90.41%; functions: 84.29% | Lines: 99.28%; regions: 99.25%; functions: 99.74% | Lines: +10.40 pp; regions: +8.84 pp; functions: +15.45 pp | [Coverage evidence](../../closed/1349-1347-add-tests-http-core/coverage-evidence.md) | -| `torrust-tracker-axum-rest-api-server` | [#2637](../2637-1347-add-tests-axum-rest-api-server/ISSUE.md) | Lines: 63.87%; regions: 63.98%; functions: 51.34% | Lines: 70.57%; regions: 69.44%; functions: 58.16% | Lines: +6.70 pp; regions: +5.46 pp; functions: +6.82 pp | [Coverage evidence](../2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md) | +| `torrust-tracker-axum-rest-api-server` | [#2637](../2637-1347-add-tests-axum-rest-api-server/ISSUE.md) | Lines: 63.87%; regions: 63.98%; functions: 51.34% | Lines: 83.87%; regions: 80.58%; functions: 79.13% | Lines: +20.00 pp; regions: +16.60 pp; functions: +27.79 pp | [Coverage evidence](../2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md) | ## Delivery Strategy @@ -319,6 +319,10 @@ For each subissue implementation, the completion policy is: - 2026-10-27 07:16 UTC - GitHub Copilot - #2637 S1 (authentication and shared responses) finished: unit-only lines 63.87% to 70.57% (46 unit tests), aggregate 87.98% to 88.97%; both tables carry the interim figures against the S1 baseline until the final measurement. +- 2026-10-28 16:39 UTC - GitHub Copilot - #2637 S2 (routes, the router layers and the health check) finished: unit-only lines + 70.57% to 83.87% (76 unit tests), aggregate 88.97% to 91.32%; both tables carry the interim figures against the S1 + baseline until the final measurement. Part of the rise comes from a `develop` change to `server.rs` (the configured + drain timeout and its test), not from S2. ## Acceptance Criteria diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index f71078aea..bbde71f95 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -9,7 +9,7 @@ github-issue: 2637 spec-path: docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md branch: "2637-1347-add-tests-axum-rest-api-server-routes" related-pr: null -last-updated-utc: "2026-10-28 16:24" +last-updated-utc: "2026-10-28 16:39" semantic-links: skill-links: - create-issue @@ -536,7 +536,7 @@ declaration-only rows have no executable entries. | ---: | --- | ---: | --- | ---: | ---: | --- | --- | --- | | 1 | `v1/middlewares/auth.rs` | 1 | S1 | 71 / 103 (68.93%) | 4 | Property candidate | DONE | [v1-middlewares-auth-tests.md](test-refactor-plans/v1-middlewares-auth-tests.md) (done; 142 / 142 lines after, 27 test cases, 8 mutants killed, property test declined) | | 2 | `v1/routes.rs` | 2 | S2 | 27 / 45 (60.00%) | 0 | Test candidate | DONE | [v1-routes-tests.md](test-refactor-plans/v1-routes-tests.md) (done; 111 / 111 lines after, 19 test cases, 6 mutants killed, private test_support helper) | -| 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | DONE | [routes-tests.md](test-refactor-plans/routes-tests.md) (done; 174 / 174 lines after, 9 test cases, 8 of 10 mutants killed, the 2 survivors recorded, R0 refactor) | +| 3 | `routes.rs` | 2 | S2 | 58 / 67 (86.57%) | 0 | Test candidate | DONE | [routes-tests.md](test-refactor-plans/routes-tests.md) (done; 169 / 169 lines after, 9 test cases, 8 of 10 mutants killed, the 2 survivors recorded, R0 refactor) | | 4 | `v1/context/health_check/handlers.rs` | 2 | S2 | 4 / 4 (100.00%) | 0 | Test candidate (small) | DONE | [v1-context-health-check-handlers-tests.md](test-refactor-plans/v1-context-health-check-handlers-tests.md) (done; 16 / 16 lines after, 1 test, mutant killed; protection not coverage) | | 5 | `v1/responses.rs` | 3 | S1 | 8 / 40 (20.00%) | 0 | Test candidate | DONE | [v1-responses-tests.md](test-refactor-plans/v1-responses-tests.md) (done; 66 / 66 lines after, 8 test cases, 8 mutants killed) | | 6 | `v1/context/auth_key/handlers.rs` | 4 | S3 | 0 / 56 (0.00%) | 0 | Test candidate | PENDING | - | @@ -692,6 +692,14 @@ once. If the slicing changes, the Slice column changes with it. (100.00%), 9 test cases, eight of ten named mutations killed (two survive by design and are recorded), and the behavior-preserving `with_layers` refactor in its own commit; the maintainer approved the completed file (16:22 UTC). Proposed the plan for row 4 (`v1/context/health_check/handlers.rs`), the last file of S2. +- 2026-10-28 16:39 UTC - GitHub Copilot - S2 close: rows 2 to 4 are `DONE` (`v1/routes.rs` 111 / 111, `routes.rs` 169 / 169 and the + health check handler 16 / 16 unit-only lines), with the behavior-preserving `with_layers` refactor in its own commit + and the crate-private `test_support` helper. Correction to the 16:24 UTC entry above: `routes.rs` is 169 / 169 lines, + not 174 / 174, because the earlier figure was measured before the timeout assertion was simplified; the ledger, + evidence and plan now say 169 / 169. Rebased onto `develop`, which changed the package meanwhile (the configured + drain timeout in `server.rs` and its test environment), and measured again: unit-only lines 1,305 / 1,556 + (83.87%), aggregate 1,421 / 1,556 (91.32%), integration-only 682 / 916 (74.45%); 76 unit tests. Updated the EPIC + "Latest" values. ## Acceptance Criteria diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md index 7e8145dc7..8ea9897b5 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/coverage-evidence.md @@ -220,8 +220,20 @@ Unit-only (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): lines ### `routes.rs` (slice S2) -Unit-only (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): lines 58 / 67 (86.57%) before, 174 / 174 (100.00%) after; regions 103 / 117 before, 280 / 280 after; functions 5 / 8 before, 26 / 26 after. The counts include the in-file test module. The request, response and failure log closures run but their content is not asserted at unit level (an integration test asserts the `ERROR` line). Nine test cases; eight of ten named mutations killed, two survive by design (see [the plan](test-refactor-plans/routes-tests.md)). +Unit-only (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): lines 58 / 67 (86.57%) before, 169 / 169 (100.00%) after; regions 103 / 117 before, 277 / 277 after; functions 5 / 8 before, 26 / 26 after. The counts include the in-file test module. The request, response and failure log closures run but their content is not asserted at unit level (an integration test asserts the `ERROR` line). Nine test cases; eight of ten named mutations killed, two survive by design (see [the plan](test-refactor-plans/routes-tests.md)). ### `v1/context/health_check/handlers.rs` (slice S2) Unit-only (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): lines 4 / 4 (100.00%) before, 16 / 16 (100.00%) after; regions 4 / 4 before, 25 / 25 after; functions 2 / 2 before, 4 / 4 after. The file was already fully covered; the single test adds protection (nothing asserted the answer), and the counts grow only by the test module. The mutation (report `Status::Error`) is killed (see [the plan](test-refactor-plans/v1-context-health-check-handlers-tests.md)). + +### Slice S2 Measurement + +After the four S2 files (`v1/routes.rs`, `routes.rs`, `v1/context/health_check/handlers.rs`, and the crate-private `test_support.rs` helper), on the S2 branch rebased onto `develop`. Stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, a clean workspace before each report, the commands of the Measurement Scope section. `develop` changed the package while S2 ran: `feat(axum-rest-api-server): apply the configured drain timeout to the REST API` (in `server.rs`, which is the S5 file, and the out-of-scope `testing/environment.rs`) added a unit test and 40 lines, so part of the rise below is not from S2. + +| Scope | Tests | Lines | Regions | Functions | +| --- | ---: | ---: | ---: | ---: | +| Aggregate (all selected test targets) | 76 unit + 58 integration | 1,421 / 1,556 (91.32%) | 2,122 / 2,405 (88.23%) | 205 / 230 (89.13%) | +| Unit-only (`--lib`) | 76 unit | 1,305 / 1,556 (83.87%) | 1,938 / 2,405 (80.58%) | 182 / 230 (79.13%) | +| Integration-only (`--test integration`) | 58 integration | 682 / 916 (74.45%) | 949 / 1,360 (69.78%) | 98 / 138 (71.01%) | + +Unit-only lines move 921 / 1,305 (70.57%) at the S2 baseline (the S1 measurement) to 1,305 / 1,556 (83.87%). The files S2 changed are fully covered by unit tests, counting their test modules: `v1/routes.rs` 111 / 111, `routes.rs` 169 / 169, `v1/context/health_check/handlers.rs` 16 / 16 and `test_support.rs` 31 / 31. The remaining uncovered production lines are in `server.rs` (S5), the context handlers and responses (S3 and S4), and the out-of-scope `testing/environment.rs`. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md index 76fe3dcdc..e8b69b247 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md @@ -146,9 +146,10 @@ default tracker; `access_tokens` and `service_binding` hide only construction. T ## Results - **Unit-only coverage** (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): `routes.rs` 58 / 67 lines - (86.57%) before; 174 / 174 lines (100.00%), 280 / 280 regions and 26 / 26 functions after (the counts include - the test module). Package unit-only lines after this file: 1,258 / 1,509 (83.37%), regions 1,838 / 2,305 - (79.74%), functions 178 / 226 (78.76%), 74 unit tests. + (86.57%) before; 169 / 169 lines (100.00%), 277 / 277 regions and 26 / 26 functions after (the counts include + the test module). An earlier version of this entry said 174 / 174 and 280 / 280: those were measured before the + timeout assertion was simplified, which removed 5 lines; the figures here come from the S2 slice measurement. + The package figures after S2 are in the slice measurement of `coverage-evidence.md`. - **Honest note on coverage:** the request, response and failure log closures (lines 67-136) are now executed, because the unauthorized request goes through the router and a `5xx` response runs the server-error and failure arms. No test asserts their content (maintainer decision: no unit-level log assertions). The `ERROR ... API ... ` From 6565adea16b5b680564f0f19fd1b278c73658429 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 16:51:59 +0000 Subject: [PATCH 12/17] test(axum-rest-api-server): [#2637] stop shadowing the response 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> --- packages/axum-rest-api-server/src/routes.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/axum-rest-api-server/src/routes.rs b/packages/axum-rest-api-server/src/routes.rs index 7078b2ef0..198db6e4f 100644 --- a/packages/axum-rest-api-server/src/routes.rs +++ b/packages/axum-rest-api-server/src/routes.rs @@ -205,7 +205,7 @@ mod tests { .expect("the request is well formed") } - async fn received(response: axum::response::Response) -> Received { + async fn into_received(response: axum::response::Response) -> Received { let status = response.status(); let headers = response.headers().clone(); let body = to_bytes(response.into_body(), usize::MAX) @@ -224,7 +224,7 @@ mod tests { let container = api_container(&ephemeral()).await; let router = router(&container, access_tokens(), &service_binding()); - received(router.oneshot(request).await.expect("the router never fails")).await + into_received(router.oneshot(request).await.expect("the router never fails")).await } #[tokio::test] @@ -243,7 +243,7 @@ mod tests { r#"Unhandled rejection: Err { reason: "token not valid" }"# )] #[tokio::test] - async fn it_should_reject_a_request_to_an_api_route_without_a_configured_token(#[case] uri: &str, #[case] body: &str) { + async fn it_should_reject_a_request_to_an_api_route_without_a_valid_token(#[case] uri: &str, #[case] body: &str) { let request = get_request(uri); let received = through_the_router(request).await; @@ -336,7 +336,7 @@ mod tests { .expect("the router answers within the deadline") .expect("the router never fails"); - let received = received(response).await; + let received = into_received(response).await; assert_eq!((received.status, received.body), (StatusCode::REQUEST_TIMEOUT, String::new())); } } From 12b9c922f0066b5c1c8299dabfe6bc8c6d52bbf8 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 16:52:47 +0000 Subject: [PATCH 13/17] docs(issues): [#2637] fix the records items of the S2 independent 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> --- .../ISSUE.md | 6 +++++- .../agent-review-reports.md | 17 +++++++++++++++++ .../test-refactor-plans/routes-tests.md | 11 +++++++---- .../v1-context-health-check-handlers-tests.md | 2 +- .../test-refactor-plans/v1-routes-tests.md | 2 +- 5 files changed, 31 insertions(+), 7 deletions(-) diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index bbde71f95..bc10ce8a3 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -9,7 +9,7 @@ github-issue: 2637 spec-path: docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md branch: "2637-1347-add-tests-axum-rest-api-server-routes" related-pr: null -last-updated-utc: "2026-10-28 16:39" +last-updated-utc: "2026-10-28 16:52" semantic-links: skill-links: - create-issue @@ -700,6 +700,10 @@ once. If the slicing changes, the Slice column changes with it. drain timeout in `server.rs` and its test environment), and measured again: unit-only lines 1,305 / 1,556 (83.87%), aggregate 1,421 / 1,556 (91.32%), integration-only 682 / 916 (74.45%); 76 unit tests. Updated the EPIC "Latest" values. +- 2026-10-28 16:52 UTC - GitHub Copilot - Added the missing approvals of row 4 (`v1/context/health_check/handlers.rs`): the maintainer approved the plan on + 2026-10-28 after the plan commit of 16:24 UTC and before its test was written (the answer to the plan question, with no + change requested), and approved the completed file at 16:35 UTC. The independent S2 review found that the log did + not record them. Also fixed the other records items of that review (see `agent-review-reports.md`). ## Acceptance Criteria diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/agent-review-reports.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/agent-review-reports.md index ef801b739..915d78f1f 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/agent-review-reports.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/agent-review-reports.md @@ -54,3 +54,20 @@ semantic-links: - Verdict: REVIEW PASSED - Follow-up actions: - None. + +### 2026-10-28 16:50 UTC - Task Reviewer (slice S2, first review) + +- Invocation scope: slice S2 of #2637 on branch `2637-1347-add-tests-axum-rest-api-server-routes`: the commits above `torrust/develop`, including `refactor(axum-rest-api-server): [#2637] separate the router layers from the routes` and the test commits for `v1/routes.rs`, `routes.rs` and the health check handler. +- Inputs: the issue folder, the EPIC, the subissue guidelines, the `write-unit-test` skill, ADR `20261021190318` rule C, and the changed Rust files. +- Evidence: the reviewer recomputed the three coverage scopes with clean `cargo llvm-cov` runs (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`) and every recorded figure matched, including the "before" figures rebuilt from a `develop` snapshot and the six EPIC deltas; the production refactor commit is a pure move (same chain and order, same 5 s default, same `#[instrument]`); eight mutations (G1, G5, L1, L5, L8, H1, L6, L10) behaved as the plans say, L6 and L10 surviving by design; `cargo +nightly fmt --all -- --check` (nightly Rust toolchain), `cargo clippy --all-targets`, `cargo machete --with-metadata`, `git diff --check`, `linter markdown` and `linter cspell` passed; no closing keyword and no cited branch SHA; every log stamp was at or before its commit. +- Findings: + - MEDIUM: the row 4 plan approval and completed-file approval were recorded in the plan index and plan but not in the progress log. + - LOW: package-level figures in two plans were measured before the rebase onto `develop` and were not labelled as such. + - LOW: the routes plan's case table expected a content type that the tests do not assert. + - LOW: the timeout test bound a variable named like the helper that reads the response (`received`). + - LOW: one rejection test name was narrower than its cases (a wrong token). + - LOW: the note on log coverage did not say that the integration test most likely protects only the server-error response event and not the `on_failure` closure. + - LOW (declined): `v1/routes.rs`'s helper returns a content type that three tests ignore; the two `409` tables assert it, so the helper stays as it is. +- Verdict: REVIEW PASSED +- Follow-up actions: + - The approvals were added to the progress log, the figures were labelled, the table and the log note were corrected, and the helper and the test were renamed: `test(axum-rest-api-server): [#2637] stop shadowing the response reader in the router tests` and `docs(issues): [#2637] fix the records items of the S2 independent review`. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md index e8b69b247..8e10b04d0 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/routes-tests.md @@ -72,7 +72,7 @@ literal `AccessTokens` (`test-token`) built in the test, not read from the confi | Request | Expected | | --- | --- | | `GET /api/health_check`, no token | `200` | -| `GET /api/v1/stats`, no token | `500`, `text/plain; charset=utf-8`, `Unhandled rejection: Err { reason: "unauthorized" }` | +| `GET /api/v1/stats`, no token | `500`, `Unhandled rejection: Err { reason: "unauthorized" }` (the content type is asserted by the S1 middleware tests) | | `GET /api/v1/stats?token=wrong` | `500`, the `token not valid` body | | `GET /api/v1/stats?token=test-token` | `200` | | `GET /api/health_check`, no `x-request-id` | response header `x-request-id` is a valid UUID (parsed with `uuid`) | @@ -133,7 +133,7 @@ Temporary prose, written before comparing with the code. | Test (cases) | Arrange prose | Act prose | Assert prose | Outcome | | --- | --- | --- | --- | --- | | `it_should_serve_the_health_check_without_a_token` | A request to the health check with no token. | Send it through the router. | `200`. | Only the status: the body belongs to the health check handler (row 4). Kept. | -| `it_should_reject_a_request_to_an_api_route_without_a_configured_token` (2) | A v1 request with no token, or with a wrong one. | Same. | `500` and the literal `unauthorized` or `token not valid` body. | The middleware's own cases were done in S1; this pins that the router applies it to v1 and feeds it the tokens. Kept. | +| `it_should_reject_a_request_to_an_api_route_without_a_valid_token` (2) | A v1 request with no token, or with a wrong one. | Same. | `500` and the literal `unauthorized` or `token not valid` body. | The middleware's own cases were done in S1; this pins that the router applies it to v1 and feeds it the tokens. Kept. | | `it_should_serve_an_api_route_when_the_request_carries_a_token_given_to_the_router` | A v1 request with the literal token that the test gave the router. | Same. | `200`. | The token is a literal in the test, not read from the configuration. Kept. | | `it_should_generate_a_request_id_when_the_request_has_none` | A request with no `x-request-id`. | Same. | The response header parses as a UUID. | The value is random, so the property is the format. Kept. | | `it_should_return_the_request_id_provided_by_the_client` | A request with `x-request-id: my-request-1`. | Same. | The response carries exactly that value. | Kept. | @@ -152,8 +152,11 @@ default tracker; `access_tokens` and `service_binding` hide only construction. T The package figures after S2 are in the slice measurement of `coverage-evidence.md`. - **Honest note on coverage:** the request, response and failure log closures (lines 67-136) are now executed, because the unauthorized request goes through the router and a `5xx` response runs the server-error and failure arms. No - test asserts their content (maintainer decision: no unit-level log assertions). The `ERROR ... API ... ` - line is asserted by an integration test, so those lines are covered, not protected, at unit level. + test asserts their content (maintainer decision: no unit-level log assertions). An integration test + (`tests/server/v1/contract/authentication.rs`) asserts one `ERROR ... API ... ` line, which most likely + matches only the server-error response event, because the `on_failure` event has no request id field. So the + response arm has an integration-level assertion, and the `on_failure` closure has no assertion at any level; at unit + level both are covered, not protected. - **Tests:** 9 test cases in seven tests (two `rstest` tables of 2). No integration test was added or changed. - **R0 refactor:** commit `refactor(axum-rest-api-server): [#2637] separate the router layers from the routes`. A pure move: the chain and its order are unchanged, `timeout` replaces the constant, and the prefix is a constant. The diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md index 18b639f06..f52104b4e 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-context-health-check-handlers-tests.md @@ -81,7 +81,7 @@ Temporary prose, written before comparing with the code. - **Unit-only coverage** (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): 4 / 4 lines (100.00%) before; 16 / 16 lines (100.00%), 25 / 25 regions and 4 / 4 functions after. The file was already fully covered, so the line gain is only - the test module: this test adds protection, not production-line coverage. Package unit-only lines after this file: + the test module: this test adds protection, not production-line coverage. Package unit-only lines after this file (before the rebase onto `develop`): 1,265 / 1,516 (83.44%), regions 1,856 / 2,323 (79.90%), functions 180 / 228 (78.95%), 75 unit tests. - **Mutation H1** (report `Status::Error`) applied alone and reverted exactly: killed by the one test. - **Tests:** one test. No integration test was added or changed. diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md index 864baf043..ca17141a7 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/test-refactor-plans/v1-routes-tests.md @@ -162,7 +162,7 @@ builds the container, composes the routes and reads the response, and hides only - **Unit-only coverage** (stable Rust 1.99.1, `cargo-llvm-cov 0.6.16`, clean workspace): `v1/routes.rs` 27 / 45 lines (60.00%) before; 111 / 111 lines (100.00%), 174 / 174 regions and 13 / 13 functions after (the counts include the test module). The new `test_support.rs` is 31 / 31. The four context `routes.rs` files stay at 100% - (their paths and methods are now pinned by the registration table). Package unit-only lines after this file: + (their paths and methods are now pinned by the registration table). Package unit-only lines after this file (before the rebase onto `develop`): 1,142 / 1,402 (81.46%), regions 1,661 / 2,142 (77.54%), functions 157 / 208 (75.48%), 65 unit tests; the denominator grew by the test module and the helper. - **Tests:** 19 test cases in six tests (single tests and `rstest` tables of 2, 2, 2 and 11 cases). No From ed79a8a339201ef220f79426c09e35485d7493ac Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 16:56:25 +0000 Subject: [PATCH 14/17] docs(issues): [#2637] record the S2 pull request number in the spec and the EPIC Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/issues/open/1347-overhaul-packages-testing/EPIC.md | 4 ++-- .../open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/issues/open/1347-overhaul-packages-testing/EPIC.md b/docs/issues/open/1347-overhaul-packages-testing/EPIC.md index d312c1bc1..bdf72310b 100644 --- a/docs/issues/open/1347-overhaul-packages-testing/EPIC.md +++ b/docs/issues/open/1347-overhaul-packages-testing/EPIC.md @@ -6,7 +6,7 @@ epic: null github-issue: 1347 spec-path: docs/issues/open/1347-overhaul-packages-testing/EPIC.md epic-owner: josecelano -last-updated-utc: "2026-10-28 16:39" +last-updated-utc: "2026-10-28 16:55" semantic-links: skill-links: - create-issue @@ -95,7 +95,7 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. | 7 | #2283 - Increase UDP server package coverage | `docs/issues/closed/2283-1347-increase-udp-server-package-coverage/ISSUE.md` | DONE | Every package source file processed through a reviewed per-file plan; unit-only lines 96.18% → 98.18%; bounded mutation sample and manual verification recorded; merged via PR #2381. | | 8 | #2301 - Review package coverage rollout | `docs/issues/open/2301-1347-review-package-coverage-rollout/ISSUE.md` | TODO | After the EPIC completes, review real rollout evidence before retaining, revising, or proposing a required coverage check. | | 9 | #2361 - Keep the report-only package coverage summary from failing on a discovery failure | `docs/issues/open/2361-1347-package-coverage-summary-discovery-failure/ISSUE.md` | TODO | Bug follow-up of #2347 (`review-finding:pr-2293-f1`): the #2222 summary check goes red with a JSON parse error when discovery fails. | -| 10 | #2637 - Add tests to the axum-rest-api-server package | `docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md` | IN_PROGRESS | S0 (spec and T1 baseline) merged through #2638; S1 (authentication and shared responses) merged through #2667; S2 (routes) is implemented and awaits its pull request; S3-S6 follow. Replaces the former draft that was misfiled under #1349. | +| 10 | #2637 - Add tests to the axum-rest-api-server package | `docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md` | IN_PROGRESS | S0 (spec and T1 baseline) merged through #2638; S1 (authentication and shared responses) merged through #2667; S2 (routes) is in review in #2687; S3-S6 follow. Replaces the former draft that was misfiled under #1349. | | 11 | Additional package-testing subissues | Create a folder-style spec when a concrete package need is identified | TODO | Permitted but not required upfront; retain scope in this EPIC. | ## Package Coverage Tracking diff --git a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md index bc10ce8a3..375f16ea1 100644 --- a/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md +++ b/docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md @@ -8,8 +8,8 @@ epic: 1347 github-issue: 2637 spec-path: docs/issues/open/2637-1347-add-tests-axum-rest-api-server/ISSUE.md branch: "2637-1347-add-tests-axum-rest-api-server-routes" -related-pr: null -last-updated-utc: "2026-10-28 16:52" +related-pr: 2687 +last-updated-utc: "2026-10-28 16:55" semantic-links: skill-links: - create-issue @@ -406,7 +406,7 @@ ledger order inside each slice is still confirmed with the first plan (T3). | --- | --- | --- | --- | --- | --- | | S0 | This specification, the T1 coverage evidence and inventory, and the EPIC registration with the baseline | - | `2637-1347-add-tests-axum-rest-api-server-spec` | #2638 | DONE | | S1 | T2-T3, then authentication and the shared response contract (ranks 1 and 3) | 1, 5 | `2637-1347-add-tests-axum-rest-api-server-auth` | #2667 | DONE | -| S2 | Route composition, transport layers and the health check (rank 2) | 2-4 | `2637-1347-add-tests-axum-rest-api-server-routes` | TBD | IN_PROGRESS | +| S2 | Route composition, transport layers and the health check (rank 2) | 2-4 | `2637-1347-add-tests-axum-rest-api-server-routes` | #2687 | IN_PROGRESS | | S3 | State-changing contexts: auth keys and whitelist (rank 4) | 6-11 | `2637-1347-add-tests-axum-rest-api-server-commands` | TBD | PENDING | | S4 | Read-only contexts: torrents and statistics (rank 5) | 12-17 | `2637-1347-add-tests-axum-rest-api-server-queries` | TBD | PENDING | | S5 | Server lifecycle (rank 6) | 18 | `2637-1347-add-tests-axum-rest-api-server-lifecycle` | TBD | PENDING | @@ -704,6 +704,7 @@ once. If the slicing changes, the Slice column changes with it. 2026-10-28 after the plan commit of 16:24 UTC and before its test was written (the answer to the plan question, with no change requested), and approved the completed file at 16:35 UTC. The independent S2 review found that the log did not record them. Also fixed the other records items of that review (see `agent-review-reports.md`). +- 2026-10-28 16:55 UTC - GitHub Copilot - Recorded S2 pull request #2687 (, `Related to #2637`, opened at 16:55 UTC) in the frontmatter, the slice table and the EPIC row. ## Acceptance Criteria From 14f4071c2461ec9a6a732c82c7a9b4998a493b8d Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 17:22:06 +0000 Subject: [PATCH 15/17] test(axum-rest-api-server): [#2637] select the first tracker without 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 #2687. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- packages/axum-rest-api-server/src/test_support.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/axum-rest-api-server/src/test_support.rs b/packages/axum-rest-api-server/src/test_support.rs index 0fe49fbc9..cda2fdc20 100644 --- a/packages/axum-rest-api-server/src/test_support.rs +++ b/packages/axum-rest-api-server/src/test_support.rs @@ -19,15 +19,17 @@ pub async fn api_container(configuration: &Configuration) -> Arc Date: Wed, 28 Oct 2026 17:23:32 +0000 Subject: [PATCH 16/17] test(axum-rest-api-server): [#2637] bound the response bodies 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 #2687. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- packages/axum-rest-api-server/src/routes.rs | 6 +++--- packages/axum-rest-api-server/src/test_support.rs | 4 ++++ .../src/v1/context/health_check/handlers.rs | 5 ++++- packages/axum-rest-api-server/src/v1/middlewares/auth.rs | 5 +++-- packages/axum-rest-api-server/src/v1/responses.rs | 5 +++-- packages/axum-rest-api-server/src/v1/routes.rs | 6 +++--- 6 files changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/axum-rest-api-server/src/routes.rs b/packages/axum-rest-api-server/src/routes.rs index 198db6e4f..4e71bb7df 100644 --- a/packages/axum-rest-api-server/src/routes.rs +++ b/packages/axum-rest-api-server/src/routes.rs @@ -177,7 +177,7 @@ mod tests { use uuid::Uuid; use super::{router, with_layers}; - use crate::test_support::api_container; + use crate::test_support::{MAX_BODY_BYTES, api_container}; const TOKEN: &str = "test-token"; const DEADLINE: Duration = Duration::from_secs(5); @@ -208,9 +208,9 @@ mod tests { async fn into_received(response: axum::response::Response) -> Received { let status = response.status(); let headers = response.headers().clone(); - let body = to_bytes(response.into_body(), usize::MAX) + let body = to_bytes(response.into_body(), MAX_BODY_BYTES) .await - .expect("the body is readable"); + .expect("the body is readable and within the limit"); Received { status, diff --git a/packages/axum-rest-api-server/src/test_support.rs b/packages/axum-rest-api-server/src/test_support.rs index cda2fdc20..eb9751c00 100644 --- a/packages/axum-rest-api-server/src/test_support.rs +++ b/packages/axum-rest-api-server/src/test_support.rs @@ -9,6 +9,10 @@ use torrust_tracker_configuration::v3_0_0::Configuration; use torrust_tracker_primitives::{ConfigurationInstanceId, ServiceRole}; use torrust_tracker_rest_api_runtime_adapter::v1::container::TrackerHttpApiCoreContainer; +/// The most bytes that a test reads from a response body. The bodies under test are small, so a larger one is a +/// regression that should fail the test instead of being buffered. +pub const MAX_BODY_BYTES: usize = 1024 * 1024; + /// Builds the REST API container for a configuration, as the application does at startup. /// /// # Panics diff --git a/packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs b/packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs index a48ec2180..710840dd4 100644 --- a/packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs +++ b/packages/axum-rest-api-server/src/v1/context/health_check/handlers.rs @@ -17,12 +17,15 @@ mod tests { use axum::response::IntoResponse; use super::health_check_handler; + use crate::test_support::MAX_BODY_BYTES; #[tokio::test] async fn it_should_answer_ok_with_a_json_report_whose_status_is_ok() { let (parts, body) = health_check_handler().await.into_response().into_parts(); - let body = to_bytes(body, usize::MAX).await.expect("the body is readable"); + let body = to_bytes(body, MAX_BODY_BYTES) + .await + .expect("the body is readable and within the limit"); assert_eq!( (parts.status, parts.headers[CONTENT_TYPE].as_bytes(), &body[..]), (StatusCode::OK, &b"application/json"[..], &br#"{"status":"Ok"}"#[..]) diff --git a/packages/axum-rest-api-server/src/v1/middlewares/auth.rs b/packages/axum-rest-api-server/src/v1/middlewares/auth.rs index 348c5bc66..5c09c1f8e 100644 --- a/packages/axum-rest-api-server/src/v1/middlewares/auth.rs +++ b/packages/axum-rest-api-server/src/v1/middlewares/auth.rs @@ -202,6 +202,7 @@ mod tests { use tower::ServiceExt; use super::{State, auth, authenticate}; + use crate::test_support::MAX_BODY_BYTES; const FIRST_TOKEN: &str = "api-token-12345"; const SECOND_TOKEN: &str = "other-token-678"; @@ -260,9 +261,9 @@ mod tests { .headers() .get(CONTENT_TYPE) .map(|value| value.to_str().expect("the content type is text").to_string()); - let body = to_bytes(response.into_body(), usize::MAX) + let body = to_bytes(response.into_body(), MAX_BODY_BYTES) .await - .expect("the body is readable"); + .expect("the body is readable and within the limit"); Sent { status, diff --git a/packages/axum-rest-api-server/src/v1/responses.rs b/packages/axum-rest-api-server/src/v1/responses.rs index 1c625aa56..8a4d75c29 100644 --- a/packages/axum-rest-api-server/src/v1/responses.rs +++ b/packages/axum-rest-api-server/src/v1/responses.rs @@ -110,6 +110,7 @@ mod tests { bad_request_response, disabled_by_configuration_response, invalid_auth_key_param_response, invalid_info_hash_param_response, ok_response, unhandled_rejection_response, }; + use crate::test_support::MAX_BODY_BYTES; const JSON: &str = "application/json"; const TEXT: &str = "text/plain; charset=utf-8"; @@ -124,9 +125,9 @@ mod tests { .to_str() .expect("the content type is text") .to_string(); - let body = to_bytes(response.into_body(), usize::MAX) + let body = to_bytes(response.into_body(), MAX_BODY_BYTES) .await - .expect("the body is readable"); + .expect("the body is readable and within the limit"); ( status, diff --git a/packages/axum-rest-api-server/src/v1/routes.rs b/packages/axum-rest-api-server/src/v1/routes.rs index ca15b1a88..bb0d97a31 100644 --- a/packages/axum-rest-api-server/src/v1/routes.rs +++ b/packages/axum-rest-api-server/src/v1/routes.rs @@ -85,7 +85,7 @@ mod tests { use tower::ServiceExt; use super::add; - use crate::test_support::api_container; + use crate::test_support::{MAX_BODY_BYTES, api_container}; const KEY_PATH: &str = "/api/v1/key/60"; const WHITELIST_RELOAD_PATH: &str = "/api/v1/whitelist/reload"; @@ -126,9 +126,9 @@ mod tests { .headers() .get(CONTENT_TYPE) .map(|value| value.to_str().expect("the content type is text").to_string()); - let body = to_bytes(response.into_body(), usize::MAX) + let body = to_bytes(response.into_body(), MAX_BODY_BYTES) .await - .expect("the body is readable"); + .expect("the body is readable and within the limit"); ( status, From b2bb60d3d2890053d2e312669351f5f7c3f8367e Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 28 Oct 2026 17:24:51 +0000 Subject: [PATCH 17/17] docs(pr-reviews): audit PR #2687 Copilot findings Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/pr-reviews/pr-2687-review/PR-REVIEW.md | 97 +++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 docs/pr-reviews/pr-2687-review/PR-REVIEW.md diff --git a/docs/pr-reviews/pr-2687-review/PR-REVIEW.md b/docs/pr-reviews/pr-2687-review/PR-REVIEW.md new file mode 100644 index 000000000..100cff6a3 --- /dev/null +++ b/docs/pr-reviews/pr-2687-review/PR-REVIEW.md @@ -0,0 +1,97 @@ +--- +semantic-links: + skill-links: + - process-pr-review + related-artifacts: + - .github/skills/dev/pr-reviews/process-pr-review/SKILL.md +--- + + + +# PR #2687 Review Audit + +Source: pull-request reviews and inline review threads for . + +## Ownership + +The PR author owns this tracked audit record. Reviewers, including repository review agents, +deliver findings through GitHub and have no repository-artifact obligation. + +- Post-merge workflow approval: N/A + +## Status Values + +- Relationship: `ORIGINAL`, `RE_RAISE_OF:` +- Disposition: `FIXED`, `NO_ACTION`, `SUPERSEDED`, `FOLLOW_UP` +- Thread state: `OPEN`, `RESOLVED`, `NON_RESOLVABLE`, `SUPERSEDED`. `OPEN` applies prospectively + to audits created or updated for approved follow-up work; historical audits remain valid without + bulk migration. +- Severity: `Blocker`, `Major`, `Minor`, `Nit`, `Suggestion`; append `(inferred)` when derived + from free prose. +- Author class: `Copilot`, `Human`, `Unknown` +- Category: `link-integrity`, `formatting`, `metadata`, `testing`, `correctness`, + `documentation`, `maintainability`, `security`, `other` +- An outdated thread whose concern was fixed is `FIXED`/`RESOLVED`, even when GitHub marks the + original thread outdated after the push. For in-PR feedback, use `NO_ACTION`/`SUPERSEDED` only + for a duplicate, superseded, or no-change concern. A post-merge `NO_ACTION` requires maintainer + approval to decline the follow-up work. + +## Findings + +Round 1 is the Copilot review `5686737218` (submitted 2026-10-28 17:13 UTC). Neither inline comment carries a +`[Severity][Id]` prefix, so the finding IDs are audit-local and the severities are inferred. + +| Finding ID | Review finding reference | Author class | Severity | Category | Relationship | Disposition | Thread state | +| ---------- | ------------------------ | ------------ | -------- | -------- | ------------ | ----------- | ------------ | +| F1 | review-finding:pr-2687-f1 | Copilot | Minor (inferred) | testing | ORIGINAL | FIXED | RESOLVED | +| F2 | review-finding:pr-2687-f2 | Copilot | Minor (inferred) | testing | ORIGINAL | FIXED | RESOLVED | + +## Finding Details + +### F1 - Bound the response bodies the test helpers read + +- PR number: 2687 +- Source review ID: 5686737218 +- Reviewer finding ID: N/A +- Source URL: https://github.com/torrust/torrust-tracker/pull/2687#discussion_r4403739901 +- Concern: A test helper read the response body with `usize::MAX` as the limit, which removes any bound on the buffered size, and the reviewer asked to apply the same change to the other helpers that do so. +- Solution: Five helpers read bodies with `usize::MAX`: three in this pull request (`routes.rs`, `v1/routes.rs`, the health check test) and two in the already merged S1 files (`v1/middlewares/auth.rs`, `v1/responses.rs`). One `MAX_BODY_BYTES` (1 MiB) now lives in the crate-private `test_support` module and all five use it, with an expect message that names the limit. +- Current-tree verification: `grep -rn 'usize::MAX' packages/axum-rest-api-server/src` finds nothing after the change. With the limit temporarily set to 1 byte, 56 unit tests failed with `LengthLimitError`; the restored value passed the 76 unit and 58 integration tests. +- Resolution reference: test(axum-rest-api-server): [#2637] bound the response bodies the tests read +- Follow-up PR URL: N/A +- Reply URL: https://github.com/torrust/torrust-tracker/pull/2687#discussion_r4403849003 + +### F2 - Select the first tracker without indexing + +- PR number: 2687 +- Source review ID: 5686737218 +- Reviewer finding ID: N/A +- Source URL: https://github.com/torrust/torrust-tracker/pull/2687#discussion_r4403739958 +- Concern: `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, and the reviewer asked for `.first()` with an explicit `expect` and no clone of the whole list. +- Solution: The helper selects the tracker with `.as_ref().and_then(|trackers| trackers.first()).expect(...)` and clones only that element, as its `# Panics` documentation says. +- Current-tree verification: Read `packages/axum-rest-api-server/src/test_support.rs` after the change; the 76 unit tests that build the container pass. +- Resolution reference: test(axum-rest-api-server): [#2637] select the first tracker without indexing in the test container helper +- Follow-up PR URL: N/A +- Reply URL: https://github.com/torrust/torrust-tracker/pull/2687#discussion_r4403849394 + +## Processing Log + +- 2026-10-28 17:13 UTC - Copilot review `5686737218` submitted with two inline findings. No human review existed at this point. +- 2026-10-28 17:23 UTC - Started audit; fixed F2 and F1 in one signed commit each and pushed the branch (the commit subjects, not ids, are cited above), replied to both threads (17:23:46 and 17:23:48 UTC) and resolved them; each resolve mutation returned `isResolved: true`. + +## Completion Rules + +- Re-derive the reply claim against the current tree before replying or resolving a thread. +- Reply on every resolvable thread before resolving it. +- For an outdated thread whose concern was fixed, record `Disposition=FIXED` and + `Thread state=RESOLVED`, even if GitHub marks the thread outdated after the push. For a + duplicate, superseded, or no-change in-PR thread, reply exactly + `Superseded by : .`, record `Disposition=NO_ACTION` and + `Thread state=SUPERSEDED`, then resolve it. A post-merge `NO_ACTION` requires maintainer + approval to decline the follow-up work. +- A consolidated PR conversation response may cover multiple review rounds only when it names + every review ID and every finding ID with its disposition and resolution reference. Record its + durable URL in each related row. +- Cite a fix by its unique Conventional Commit subject or durable reply URL, never by a branch SHA + that can change after a rebase. +- Refresh review threads using GraphQL and confirm that no unresolved actionable thread remains.