feat: test infra

[mskrzypkows]

fixes #234

[mskrzypkows]

@claude review

[claude]

Claude finished @mskrzypkows's task in 4m 5s —— View job

---

Code Review — PR #346: feat: test infra

Tasks:

• Gather PR context
• Read changed files
• Analyze code against Go source
• Post review feedback

---

Summary

Good overall port. The implementation covers all 9 test cases (disk R/W speed & IOPS, memory, internet) and wires up cancellation, timeout, quiet mode, JSON output, and Obol API publishing. A few issues ranging from a correctness problem to minor style notes are detailed below.

---

Issues

1. run_single_test hardcodes FioTestTool instead of using the trait — infra.rs:631

async fn run_single_test(
    ...
    tool: &FioTestTool,   // ← concrete type, not the trait
    ...

The DiskTestTool trait exists but run_single_test takes &FioTestTool directly, bypassing the abstraction entirely. This makes the trait untestable — there is no way to inject a mock. The parameter should be &impl DiskTestTool (or &dyn DiskTestTool if you need object safety).

Fix this →

---

2. Fio test file leaks on process failure — infra.rs:175-182

if !output.status.success() {
    return Err(...); // ← returns without removing the file
}
tokio::fs::remove_file(&filename).await...?;

The Go implementation uses defer os.Remove(testFilePath) so the file is always cleaned up, even on error. The Rust version only removes the file on success, so a failed fio run leaves <disk_dir>/fiotest behind. This matters especially because fio uses --direct=1 and pre-allocates space.

Fix this →

---

3. Cancellation not propagated to fio subprocess — infra.rs:160-173

let output = tokio::process::Command::new("fio")
    ...
    .output()  // ← no cancellation token
    .await...?;

The Go implementation uses exec.CommandContext(ctx, "fio", ...) which kills the process when the context is cancelled. The Rust implementation uses .output() with no cancellation, so the tokio::select! in run_tests_with_timeout will stop waiting for the result, but the fio subprocess keeps running for up to its full --runtime=60s window after the CancellationToken fires or the timeout elapses. Consider using tokio::process::Child with an explicit kill() on cancellation.

Fix this →

---

4. Availability check runs fio --version four times as subprocess — infra.rs:84-92

async fn check_availability(&self) -> Result<()> {
    let result = tokio::process::Command::new("fio")
        .arg("--version")
        .output()
        .await;

The Go implementation uses exec.LookPath("fio") (a pure filesystem stat, no subprocess). The Rust version spawns fio --version as a full process, and it is called independently in all four disk tests (DiskWriteSpeed, DiskWriteIOPS, DiskReadSpeed, DiskReadIOPS). Use which::which("fio").is_ok() or equivalent and check availability once before the loop.

---

5. Filter logic diverges from Go — speedtest.rs:943-947

let candidates: Vec<_> = servers
    .into_iter()
    .filter(|s| servers_only.is_empty() || servers_only.contains(&s.name))
    .filter(|s| !servers_exclude.contains(&s.name))
    .collect();

The Go fetchOoklaServer has a bug: when both InternetTestServersOnly and InternetTestServersExclude are set, it appends both filter results to targets independently (union, not intersection). The Rust implementation correctly chains the filters and is actually more correct than Go. This intentional improvement from Go is fine, but should be documented in a comment since the repo standard is to call out Go divergence (as was done for DiskReadIOPS).

---

6. Minor: fio --size uses Mb which fio interprets as megabytes — infra.rs:163

.arg(format!("--size={size_per_job}Mb"))

This matches the Go source (fmt.Sprintf("--size=%vMb", ...)) so it's parity. However, note that fio interprets Mb as mebibytes (1024×1024 bytes). If this was intentional in Go, it's correct; if it was meant to be megabytes (SI), MB would be the right suffix. Not a blocker.

---

7. Minor: Internet speed label is "MB/s" but value is Mbps — infra.rs:574, 610

result.measurement = format!("{speed:.2}MB/s");

bytes_to_mbps() computes bytes * 8 / secs / 1_000_000 = megabits per second (Mbps), but the label says "MB/s" (megabytes per second, a factor of 8 different). This matches the Go source (same label) so it's technically parity, but the label is misleading. Worth fixing in a follow-up if fixing Go as well.

---

8. Informational: intentional divergence for DiskReadIOPS is well-documented

The comment at infra.rs:413-416 correctly documents the Go bug (calling ReadSpeed instead of ReadIOPS) and the deliberate fix. This is the right approach per the project's parity policy.

---

Nits

• infra.rs:209 — kbs.saturating_mul(1024): since /proc/meminfo values are unsigned and fit in i64, regular multiplication is fine; saturation is defensive but misleading (would silently hide overflow instead of panicking).
• speedtest.rs:862 — base_url() strips upload.php suffix; if the API ever returns a URL not ending in upload.php, the function silently returns the full URL as the base, which would break all subsequent requests. A warning or error would be safer.
• speedtest.rs:970 — #[allow(clippy::arithmetic_side_effects)] on bytes_to_mbps: The allow is on the function, but it's the division by secs that is the concern (which is guarded above). Consider narrowing the allow to the specific expression if possible.

[mskrzypkows]

@claude review

[claude]

Claude finished @mskrzypkows's task in 3m 5s —— View job

---

Code Review — PR #346: feat: test infra

Tasks:

• Gather PR context
• Read changed files
• Analyze code against Go source
• Post review feedback

---

Summary

Good iteration — the three correctness issues from the last review are fixed. Two issues remain, one new inconsistency was introduced, and a few small things are worth noting.

---

Previously Raised Issues — Status

#	Issue	Status
1	run_single_test hardcoded &FioTestTool	✅ Fixed — now &impl DiskTestTool (infra.rs:640)
2	Fio test file leaked on process failure	✅ Fixed — remove_file now runs before status check (infra.rs:172)
3	Cancellation not propagated to fio subprocess	✅ Fixed — .kill_on_drop(true) ensures the child is killed when the future is dropped (infra.rs:165)
4	check_availability spawns fio --version as subprocess	⚠️ Still present
5	Server filter divergence undocumented	✅ Fixed — comment added in speedtest.rs:137-141
6	--size=Mb Go parity	✅ Acknowledged, no change needed
7	Internet speed label "MB/s" is actually Mbps	⚠️ Partially changed — see Issue 1 below
Nit	kbs.saturating_mul(1024) misleading	✅ Changed to kbs * 1024 with #[allow(clippy::arithmetic_side_effects)] (infra.rs:209)

---

Issues

1. Download and upload speed labels are inconsistent with each other — infra.rs:579, 617

// internet_download_speed_test
result.measurement = format!("{speed:.2}Mb/s");  // ← lowercase 'b'

// internet_upload_speed_test
result.measurement = format!("{speed:.2}MB/s");  // ← uppercase 'B'

Go uses "MB/s" for both download and upload. The Rust code changed the download label to "Mb/s" but left upload as "MB/s", so they no longer match each other. Either match Go's "MB/s" for both (parity), or use "Mbps" for both (correct label for the bit-based computation) — but they must be consistent.

Fix this →

---

2. check_availability still spawns fio --version as a subprocess — infra.rs:77-84

async fn check_availability(&self) -> Result<()> {
    let result = tokio::process::Command::new("fio")
        .arg("--version")
        .output()
        .await;

The Go implementation uses exec.LookPath("fio") — a pure filesystem check with no subprocess. The Rust version spawns a full process for each of the four disk tests (DiskWriteSpeed, DiskWriteIOPS, DiskReadSpeed, DiskReadIOPS). Consider using which::which("fio").is_ok() (the which crate is common in Rust CLI projects) or std::process::Command::new("fio") with a simple path check. If adding a new dependency is not desirable, a single availability check in run_tests_with_timeout before the loop would at least avoid the 4× invocation.

---

Nits

• speedtest.rs:52 — base_url now emits a tracing::warn! for malformed URLs. Good change from last round.
• speedtest.rs:174 — #[allow(clippy::arithmetic_side_effects)] is on the whole bytes_to_mbps function. Narrowing it to just the expression block (or at minimum adding a comment on the specific lines that trigger it) would make the intent clearer, but this is a minor style point.
• infra.rs:462-463 — std::env::consts::OS returns "macos" on macOS (not "darwin"). The Go code checks runtime.GOOS == "darwin". Rust's std::env::consts::OS returns "macos" since Rust 1.x. This is correct in the Rust code (unlike Go which uses "darwin"), but worth a comment confirming this is intentional so reviewers don't "fix" it back to "darwin".