test: mocking harness + $HOME isolation + publish coverage (#447) - #504
test: mocking harness + $HOME isolation + publish coverage (#447)#5040xGaspar wants to merge 1 commit into
Conversation
Closes the remaining scope of #447 (PR #463 shipped the config/$HOME subset). Harness (src/test_support.rs, unit tests): - MockHttpServer: dependency-free loopback HTTP server (127.0.0.1:0, thread-per-connection). Register routes with `on(method, path, MockResponse)`, point the code under test at `server.url()`, assert on `server.requests()` (method, path, query, headers, body). `fallback` covers unregistered routes. - dead_port_url(): a closed loopback port for connection-refused paths. - GithubBaseGuard: thread-local redirection of the GitHub REST base and the git remote base, so the real publish/API call paths run offline. - GitIdentityGuard: git author/committer identity plus GIT_CONFIG_GLOBAL / GIT_CONFIG_NOSYSTEM, so commit-producing helpers never read ~/.gitconfig. Harness (tests/common/mod.rs, integration tests): - TempEnv: spawns fledge with fresh HOME/XDG_CONFIG_HOME/FLEDGE_CONFIG_DIR, non-interactive mode, every provider API key stripped, and OLLAMA_HOST pointed at a closed loopback port. Coverage: - publish.rs: the tautological tests and the empty `#[ignore] publish_live` stub are replaced by 20 real tests β get_authenticated_user, check_repo_exists (200/404/5xx), create_github_repo (body, org vs user URL, 422, 403), set_repo_topic (additive, no-duplicate, fetch failure), push_directory against a local bare repo (init+commit+push, re-push branch, no token in .git/config, git-error surfacing), run_publish orchestration (create/skip-create/abort), and resolve_owner. - github.rs: github_api_get against the mock server β headers, encoded query, 404/403/generic status mapping, non-JSON body, unreachable host, and a search payload decoded through search::parse_search_response. - llm.rs: OllamaProvider::invoke β request shape, Bearer auth, HTTP-status error text, undecodable body, connection refused, OLLAMA_HOST hint. - doctor.rs: probe_ollama_host true/false paths; tests/doctor.rs now runs under TempEnv (it previously probed whatever endpoint the developer's real config named) with two added assertions. - tests/main.rs: the two `review` cases run under TempEnv. Production seams (behavior-preserving, release builds unchanged): - github::api_base() resolves the REST base; publish::remote_base()/remote_url() resolve the push remote. Both return the previous constants unless a #[cfg(test)] thread-local override is set. No test contacts the network or reads/writes the real ~/.config/fledge. Specs updated: publish (v6), github (v4), and the publish/github/llm/doctor testing plans; CONTRIBUTING gains a "no network, no real $HOME" testing section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KuhwrJF2XFDVHhX7qzDuyy
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
β Corvin says...
_
<(;\ .oO(oh no...)
|/(\
\(\\
" "\\
"Caw... your imports are all over the place."
CI Summary
| Check | Status |
|---|---|
| Dependency Audit | β Passed |
| Integration (3 OS) | β skipped |
| Lint (fmt + clippy) | β Passed |
| Spec Validation | β failure |
| Tests (3 OS) | β failure |
Powered by corvid-pet
0xLeif
left a comment
There was a problem hiding this comment.
Review β REQUEST CHANGES
This is a strong test-infra PR. The harness design (dependency-free loopback MockHttpServer, thread-local GithubBaseGuard, GitIdentityGuard, TempEnv) matches fledge's blocking ureq stack well, and the production seams (github::api_base(), publish::remote_base()) are correctly #[cfg(test)]-gated so release builds stay on the production constants. Specs/testing plans were updated honestly. macOS + Ubuntu tests pass.
Blocker 1: Windows unit test failure (real bug)
test (windows-latest) fails on a single test:
publish::tests::push_directory_initializes_commits_and_pushes
assertion failed: config.contains(remotes.path().to_str().unwrap())
at src/publish.rs:786
988 passed / 1 failed. The assertion that .git/config contains the raw Windows path from Path::to_str() is brittle: git on Windows typically normalizes remotes to forward slashes (and sometimes a different drive prefix form). The push itself succeeded (the bare-repo log / ls-tree asserts above it would have failed otherwise); only the path-string check is wrong.
Suggested fix: assert via git remote get-url origin (or normalize both sides to / / PathBuf compare) instead of substring-matching the OS path in the config file text. Apply the same care anywhere else you assert path-in-config.
Blocker 2: SpecSync β no active change covering the production paths
spec-check / trust fail with:
meaningful changed paths are not covered by an active change:
src/doctor.rs, src/github.rs, src/llm.rs, src/publish.rs,
src/test_support.rs, tests/common/mod.rs, tests/doctor.rs, tests/main.rs
Plus stale accepted-change errors from editing CHANGELOG.md and specs/doctor/testing.md after those inputs were locked by CHG-0001 / CHG-0004 / CHG-0006.
Please add an active SpecSync change for this work (or attach it to one), and reopen/succeed the staled accepted changes per the specsync change reopen β¦ guidance in the log.
Non-blocking notes (optional follow-ups)
- Hand-rolled HTTP mock is the right call over wiremock/tokio here; please keep
MockHttpServerdocumented in CONTRIBUTING (already done β good). GithubBaseGuard::apisnapshots the current remote override but does not clear it; that is fine with thread-locals + RAII as long as every test that sets remote usesapi_and_remote(or a dedicated remote-only guard). Worth a one-line comment if you touch the file again.
Happy to re-review once Windows is green and the SpecSync contract passes.
Addresses the remaining scope of #447 β the mocking harness and publish-flow isolation. (The
$HOME-isolation subset landed earlier in #463, which auto-closed the issue prematurely; this does not redo that work.)The harness
Unit tests β
src/test_support.rsMockHttpServer::start()β loopback server on127.0.0.1:0, thread-per-connection, shuts down on drop.server.on("GET", "/user", MockResponse::json(200, β¦)), then assert againstserver.requests()βRecordedRequest { method, path, query, headers, body }.dead_port_url()β closed loopback port for connection-refused paths.GithubBaseGuard::api(url)/::api_and_remote(...)β thread-local redirection of the GitHub REST base and git remote base, so parallel tests can't see each other's redirection.GitIdentityGuardβ git identity plusGIT_CONFIG_GLOBAL/GIT_CONFIG_NOSYSTEM, so commit-producing code never reads~/.gitconfig.Integration tests β
tests/common/mod.rs:TempEnvspawnsfledgewith freshHOME/XDG_CONFIG_HOME/FLEDGE_CONFIG_DIR,FLEDGE_NON_INTERACTIVE=1, all ten provider API keys plusGITHUB_TOKEN/GH_TOKENremoved, andOLLAMA_HOSTpointed at a closed loopback port.Note on
wiremock: deliberately not used. It would pull in tokio, while all fledge HTTP is blockingureq. The hand-rolled server adds no dependencies.Coverage added
src/publish.rs(the explicit ask) β the tautological tests and the empty#[ignore] publish_livestub are gone, replaced by ~20 real tests:get_authenticated_user,check_repo_exists200/404/5xx,create_github_repo(body fields, user vs org endpoint, 422, 403),set_repo_topic(additive merge, no duplicate, fetch failure),push_directoryagainst a local bare repo (init+commit+push, re-push, no token leaked into.git/config, git-stderr surfacing), andrun_publishorchestration.src/github.rsβgithub_api_getend to end: headers, percent-encoded query, 404/403/generic status mapping, non-JSON body, unreachable host.src/llm.rsβOllamaProvider::invoke: request shape, Bearer auth vs none, HTTP-status errors, undecodable body, connection refused, and theOLLAMA_HOSThint (OLLAMA_HOSTenv var overrides config without clear feedback in error messagesΒ #378).src/doctor.rs+tests/doctor.rsβ the CLI doctor tests previously probed whatever endpoint the developer's real config named; they now run underTempEnv.Production seams introduced
Behavior-preserving; release builds are byte-identical.
github::api_base()β was an inlinehttps://api.github.comliteral, now apub(crate)fn with a#[cfg(test)]-only thread-local override.publish::remote_base()/remote_url(owner, repo)β same treatment for the inline remote URL. This is what makesrun_publishtestable end to end against a local bare repo.No network, no real
$HOMEEvery new HTTP call targets
127.0.0.1(mock server or closed port); git operations use tempdir working trees and local bare remotes withGIT_CONFIG_GLOBALneutered; config access goes throughFLEDGE_CONFIG_DIR/HOMEtempdirs. This rests on a full diff audit rather than a network namespace, which the sandbox blocked.Verification
cargo fmt --checkandcargo clippy --all-targets -- -D warningsclean;cargo testpasses (999 unit + all integration binaries).Specs updated:
publishv5βv6 andgithubv3βv4 (new invariants for the base-URL indirection), plus thepublish/github/llm/doctortesting plans β the publish plan previously listed tests that did not exist.fledge spec checkis red onmainfor unrelated reasons (six stale SDD change-record errors β CorvidLabs/spec-sync#481); this PR neither adds to nor fixes that.π€ Generated with Claude Code
https://claude.ai/code/session_01KuhwrJF2XFDVHhX7qzDuyy