feat(tests): add main-app integration test foundation - #2034
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements infrastructure to support multiple main app-level integration test binaries running concurrently by isolating configuration and storage per test suite and using ephemeral ports discovered via the app container registar.
Changes:
- Introduces
tests/common/helpers to create an isolated temp workspace, start the app with a per-suite config file, and discover bound HTTP endpoints. - Updates the global stats integration test suite to use port
0and an isolated workspace (no fixed ports / shared DB paths). - Adds scaffolding documentation and sample test binary (
tests/scaffold.rs) plus issue/spec docs describing the adopted execution model.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/stats.rs | Switches the suite entry point to stats and adds shared common module wiring. |
| tests/servers/api/contract/stats/mod.rs | Updates the stats scenario to start from an isolated workspace, discover ports via registar, and issue announces/stat queries. |
| tests/scaffold.rs | Adds a second integration-test binary to demonstrate parallel execution of suites. |
| tests/common/mod.rs | New shared infra for temp config+storage, startup, and registar-based endpoint discovery. |
| tests/AGENTS.md | Documents what belongs in main-level integration tests and the intended execution model. |
| docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md | Adds the issue spec/decision record for the chosen parallel test model. |
| docs/issues/drafts/increase-main-app-integration-test-coverage.md | Adds a draft tracking issue for expanding main-level integration test coverage. |
| Cargo.toml | Adjusts dev-dependencies for the new test infrastructure. |
| Cargo.lock | Locks dependency changes from the dev-dependency update. |
Comments suppressed due to low confidence (2)
tests/common/mod.rs:114
http_api_urlcurrently matches the first HTTP service bound to any loopback IP. Since the test config also starts a health-check HTTP server on127.0.0.2, this can nondeterministically return the health-check URL instead of the REST API URL.
pub async fn http_api_url(container: &AppContainer) -> Option<String> {
let reg = container.registar.entries();
let map = reg.lock().await;
map.keys()
.find(|b| b.protocol() == torrust_net_primitives::service_binding::Protocol::HTTP && b.bind_address().ip().is_loopback())
.map(|b| format!("http://{}", b.bind_address()))
}
docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md:345
- Another reference to
tests/integration.rshere conflicts with the current suite entry point (tests/stats.rs). Keeping the naming consistent will make the decision log easier to follow.
The original scope, implementation plan, acceptance criteria, and verification plan above are
superseded where they require parallel tracker application instances or multiple independently
bootstrapped test functions in `tests/integration.rs`.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let api_url = common::http_api_url(&app_container).await.expect("expected an HTTP API URL"); | ||
|
|
||
| // ── 3. Announce to both tracker instances ──────────────────────── | ||
| let client = reqwest::Client::new(); |
| let api_url = common::http_api_url(&app_container).await.expect("expected an HTTP API URL"); | ||
|
|
||
| // ── 4. Scenario: announce to both trackers ─────────────────────── | ||
| let client = reqwest::Client::new(); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2034 +/- ##
========================================
Coverage 81.46% 81.46%
========================================
Files 344 344
Lines 24601 24601
Branches 24601 24601
========================================
+ Hits 20041 20042 +1
Misses 4252 4252
+ Partials 308 307 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
tests/AGENTS.md:90
- The documented "Current Test Structure" references
tests/integration.rs, but this PR usestests/stats.rsas the main suite entry point (and there is notests/integration.rs). This makes the guidance misleading for contributors.
tests/
├── AGENTS.md # This file
├── common/
│ └── mod.rs # Shared test utilities (temp config, port extraction)
├── integration.rs # Global statistics suite (main integration tests)
├── scaffold.rs # Scaffolding demo — pattern reference for new binaries
└── servers/
└── api/
tests/servers/api/contract/stats/mod.rs:71
- The test uses
reqwest::Client::new()with no request timeout. If a request hangs (e.g., service not ready or a regression that stalls responses), CI can hang indefinitely. Setting a small client timeout makes failures deterministic.
let client = reqwest::Client::new();
for url in &tracker_urls {
let announce_url = url
.join("/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&ip=127.0.0.1&event=started&compact=0")
.expect("announce URL should be valid");
let resp = client.get(announce_url.as_str()).send().await.unwrap();
tests/scaffold.rs:110
- Same as the main stats suite:
reqwest::Client::new()has no timeout, which can cause the integration-test binary to hang on stalled HTTP requests. Use a small timeout to keep CI failures bounded.
let client = reqwest::Client::new();
tests/common/mod.rs:114
http_api_urlselects the first HTTP + loopback binding from the registar. Both the REST API (127.0.0.1) and the health-check API (127.0.0.2) are loopback, so this can nondeterministically return the health-check URL depending on map iteration order, causing flakey tests or wrong requests.
pub async fn http_api_url(container: &AppContainer) -> Option<Url> {
let reg = container.registar.entries();
let map = reg.lock().await;
map.keys()
.find(|b| b.protocol() == torrust_net_primitives::service_binding::Protocol::HTTP && b.bind_address().ip().is_loopback())
.map(|b| loopback_url(b.bind_address()))
}
tests/common/mod.rs:93
- The doc comment says both the REST API and health-check API bind to
127.0.0.1, but the test configs bindhealth_check_apito127.0.0.2. This mismatch makes the discovery logic harder to reason about (and is directly relevant to whyhttp_api_urlmust not match all loopback IPs).
/// HTTP trackers bind to `0.0.0.0` (unspecified). The REST API and health
/// check bind to `127.0.0.1` (loopback). We identify trackers by their
/// unspecified IP, which is deterministic regardless of hash-map ordering.
/// Wildcard addresses are converted to `127.0.0.1` for client requests.
tests/servers/api/contract/stats/mod.rs:84
- This comment implies dropping
JobManagercleans up the running tracker.JobManagerdoes not implementDropand dropping it does not cancel jobs; the tasks keep running until the tokio runtime for this test binary is torn down. The comment should reflect the actual shutdown behavior to avoid misleading future test authors.
// The tracker application and its temporary workspace are cleaned up
// when `workspace` and `_jobs` are dropped at the end of this scope.
tests/scaffold.rs:128
- Same as in
tests/servers/api/contract/stats/mod.rs: droppingJobManagerdoes not stop background jobs. This comment should describe the real shutdown model (runtime teardown aborts tasks) so the scaffolding sample doesn't teach an incorrect pattern.
// The tracker application and its temporary workspace are cleaned up
// when `workspace` and `_jobs` are dropped at the end of this scope.
…ructure Issue spec for torrust#1419 covering: - Three problems: logging, port conflicts, config isolation - Temp directory pattern for complete test isolation - 8-task implementation plan with prove-then-fix strategy - 9 acceptance criteria and verification plan
Tracking issue for expanding main application-level integration tests. Documents the three-layer testing strategy (unit/integration/E2E) and lists 14 prioritized tests that require full application context.
Documents what belongs in main-level vs package-level tests, infrastructure requirements (port 0, temp workspaces), and references the draft issue for future test coverage expansion.
- Replace tests/integration.rs with tests/stats.rs (rename) - Create tests/common/mod.rs with shared helpers (EphemeralTrackerWorkspace, port discovery via registar IP-based filtering) - Create tests/scaffold.rs as a demo binary for future contributors - Convert stats tests to single-suite with cross-instance aggregation focus - Update tests/AGENTS.md with the new execution model - Remove unused dev-dependencies (torrust-info-hash, torrust-tracker-client, torrust-tracker-http-protocol) - Remove obsolete tests/helpers.rs
2198e69 to
a1f7a75
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
tests/common/mod.rs:85
- A fixed 500ms sleep after
app::run()is a common source of flaky integration tests on slow/loaded CI machines. Prefer waiting for a concrete readiness condition (e.g., registar has at least one entry) with a bounded timeout instead of sleeping a fixed duration.
let (container, jobs) = app::run().await;
// Allow spawned tasks (registar insertions, listener binds) to
// complete before we attempt to read the registar or make requests.
tokio::time::sleep(std::time::Duration::from_millis(500)).await;
(container, jobs)
tests/AGENTS.md:90
- The documented tree mentions
tests/integration.rs, but this PR's integration test binary istests/stats.rs(andintegration.rsdoes not exist). This makes the guidelines misleading for contributors.
│ └── mod.rs # Shared test utilities (temp config, port extraction)
├── integration.rs # Global statistics suite (main integration tests)
├── scaffold.rs # Scaffolding demo — pattern reference for new binaries
└── servers/
└── api/
tests/common/mod.rs:16
- The doc link points to a non-existent file (
...main-app-level.md). The issue spec lives under the folder-based path ending in/ISSUE.md, so this reference is currently broken.
//! See `docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md`
//! for the full decision record.
tests/common/mod.rs:98
- This comment claims the REST API and health check bind to
127.0.0.1, but the test configs in this PR use127.0.0.1for the REST API and127.0.0.2for the health check API. Keeping this aligned matters because these helpers are already relying on bind-IP conventions for discovery.
/// HTTP trackers bind to `0.0.0.0` (unspecified). The REST API and health
/// check bind to `127.0.0.1` (loopback). We identify trackers by their
/// unspecified IP, which is deterministic regardless of hash-map ordering.
/// Wildcard addresses are converted to `127.0.0.1` for client requests.
tests/servers/api/contract/stats/mod.rs:74
- The announce request construction (including the long percent-encoded query) is duplicated in multiple places (both tests in this module and again in
tests/scaffold.rs). This makes future changes (e.g., adding params, changing info_hash/peer_id) error-prone and harder to keep consistent.
let client = reqwest::Client::new();
for url in &tracker_urls {
let announce_url = url
.join("/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&ip=127.0.0.1&event=started&compact=0")
.expect("announce URL should be valid");
|
ACK e3a4cd6 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
tests/common/mod.rs:122
http_api_urlcurrently matches any loopback HTTP service (is_loopback()), but the test configs in this PR deliberately separate the REST API (127.0.0.1) from the health-check API (127.0.0.2). Tightening the predicate to127.0.0.1makes the selection deterministic if additional loopback HTTP bindings are ever registered.
map.keys()
.find(|b| b.protocol() == torrust_net_primitives::service_binding::Protocol::HTTP && b.bind_address().ip().is_loopback())
.map(|b| loopback_url(b.bind_address()))
tests/common/mod.rs:16
- The module doc comment references a non-existent file
docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md. The decision record lives under the issue-spec folder as.../ISSUE.md, so this link is currently misleading/broken.
//! See `docs/issues/open/1419-allow-multiple-integration-tests-at-main-app-level.md`
//! for the full decision record.
tests/common/mod.rs:98
- These docs claim the REST API and health check bind to
127.0.0.1, but the test configurations in this PR bind the health-check API to127.0.0.2. Updating this comment keeps the helper documentation aligned with the actual conventions used by the tests.
This issue also appears on line 120 of the same file.
/// HTTP trackers bind to `0.0.0.0` (unspecified). The REST API and health
/// check bind to `127.0.0.1` (loopback). We identify trackers by their
/// unspecified IP, which is deterministic regardless of hash-map ordering.
/// Wildcard addresses are converted to `127.0.0.1` for client requests.
Summary
Establishes the main-application integration-test foundation for #1419 and documents the two prerequisites discovered while exercising it.
Included
tests/stats.rs, giving the suite one application instance per Cargo integration-test executable.EphemeralTrackerWorkspacehelper that creates per-test configuration and storage, and uses port zero for listener allocation.tests/scaffold.rsexample binary and updates integration-test contributor guidance.Urlrather thanStringfor runtime listener URLs in test helpers.0.0.0.0:0bootstrap defect: one disabled and one enabled tracker should contribute one global announce, but the current implementation reports two because both listeners inherit the later configuration.Deliberate Deferral
This PR does not implement #2035 or #2036. The integration helpers temporarily classify services by bind IP and wait briefly for asynchronous registration. That workaround is documented and remains acceptable only until the prerequisite issues land. #1419 stays open and resumes after them.
Validation
linter allcargo test --test statscargo +nightly fmt --checkcargo +nightly check --tests --benches --examples --workspace --all-targets --all-featurescargo +nightly doc --no-deps --bins --examples --workspace --all-featurescargo +stable test --tests --benches --examples --workspace --all-targets --all-featuresThe known #2035 regression was run explicitly and fails as expected: global
tcp4_announces_handledis2rather than the required1. It remains#[ignore]until #2035 is fixed.