sledtools/pika branch #121

pika-git-test-cleanup-3

pika-git: drop test-only legacy config scaffolding

Target branch: master

Merge Commit: 545f9e5224e6c4d003dc678377aaff85f05c1382

branch: merged tutorial: ready ci: success

Open CI Details

Continuous Integration

CI: success

Compact status on the review page, with full logs on the CI page.

Open CI Details

Latest run #148 success

2 passed

head 277bf88c0a4723bacb8ed5b805f2399eb99f1dd1 · queued 2026-03-26 21:59:59 · 2 lane(s)

queued 7s · ran 25s

check-notifications · success check-agent-contracts · success

Summary

This branch removes test-only legacy configuration scaffolding from the pika-git crate by eliminating three #[cfg(test)] fields from the Config struct (repos, merged_lookback_hours, allowed_npubs) and replacing approximately 30 inline Config { ... } literal constructions scattered across test modules with calls to two new test-helper constructors: Config::test_defaults() and Config::test_with_forge_repo(). The web module also gains two local helpers (forge_test_config_without_admins and forge_test_repo_config) to further reduce boilerplate. The net effect is a significant reduction in test maintenance surface area—when Config fields change in the future, only the two constructors need updating rather than dozens of call sites.

Tutorial Steps

Remove test-only fields from the Config struct

Intent: Eliminate three fields (`repos`, `merged_lookback_hours`, `allowed_npubs`) that were gated behind `#[cfg(test)]` and only existed to satisfy inline struct literals in tests. These fields had no production use and forced every test to supply values for them.

Affected files: crates/pika-git/src/config.rs

Evidence

@@ -21,9 +21,6 @@
 #[derive(Debug, Clone, Deserialize)]
 #[allow(dead_code)]
 pub struct Config {
-    #[cfg(test)]
-    #[serde(default)]
-    pub repos: Vec<String>,

@@ -34,9 +31,6 @@
     pub github_token_env: String,
-    #[cfg(test)]
-    #[serde(default)]
-    pub merged_lookback_hours: u64,

@@ -47,9 +41,6 @@
     pub bind_port: u16,
-    #[cfg(test)]
-    #[serde(default)]
-    pub allowed_npubs: Vec<String>,

Three fields on Config were annotated with #[cfg(test)] and #[serde(default)]:

repos: Vec<String> — a leftover from an earlier multi-repo design
merged_lookback_hours: u64 — unused outside of struct literals in tests
allowed_npubs: Vec<String> — superseded by the persistent allowlist in the store

All three are removed outright. This shrinks the production-compiled struct and eliminates the need for tests to populate these fields.

Add test-only constructor methods to Config

Intent: Provide a single source of truth for constructing `Config` values in tests, so that future field additions or defaults only need updating in one place.

Affected files: crates/pika-git/src/config.rs

Evidence

@@ -165,6 +156,32 @@ fn default_retry_backoff_secs() -> u64 {
+#[cfg(test)]
+impl Config {
+    pub(crate) fn test_defaults() -> Self {
+        Self {
+            forge_repo: None,
+            poll_interval_secs: DEFAULT_POLL_INTERVAL_SECS,
+            model: DEFAULT_MODEL.to_string(),
+            api_key_env: DEFAULT_API_KEY_ENV.to_string(),
+            github_token_env: DEFAULT_GITHUB_TOKEN_ENV.to_string(),
+            worker_concurrency: DEFAULT_WORKER_CONCURRENCY,
+            retry_backoff_secs: DEFAULT_RETRY_BACKOFF_SECS,
+            webhook_secret_env: DEFAULT_WEBHOOK_SECRET_ENV.to_string(),
+            bind_address: DEFAULT_BIND_ADDRESS.to_string(),
+            bind_port: DEFAULT_BIND_PORT,
+            bootstrap_admin_npubs: Vec::new(),
+        }
+    }
+
+    pub(crate) fn test_with_forge_repo(forge_repo: ForgeRepoConfig) -> Self {
+        Self {
+            forge_repo: Some(forge_repo),
+            ..Self::test_defaults()
+        }
+    }
+}

A #[cfg(test)] impl block is added to Config with two pub(crate) constructors:

test_defaults() — returns a Config populated entirely from the module's DEFAULT_* constants and empty collections. No forge repo is configured.
test_with_forge_repo(forge_repo) — wraps test_defaults() and sets forge_repo: Some(...), which is the common case for CI and poller tests.

Both methods use the same default constants that serde would apply during deserialization, keeping test configs consistent with real-world defaults. The pub(crate) visibility ensures they are accessible from sibling modules (ci, web, worker, etc.) but not from external crates.

Replace inline Config literals in CI tests

Intent: Collapse seven identical 15-line `Config { ... }` blocks in `ci.rs` test functions into one-liner calls to `Config::test_with_forge_repo()`.

Affected files: crates/pika-git/src/ci.rs

Evidence

@@ -870,22 +870,7 @@
-        let config = Config {
-            repos: vec!["sledtools/pika".to_string()],
-            forge_repo: Some(forge_repo.clone()),
-            ...
-        };
+        let config = Config::test_with_forge_repo(forge_repo.clone());

@@ -1025,22 +1010,7 @@
+        let config = Config::test_with_forge_repo(forge_repo.clone());

@@ -1176,22 +1146,7 @@
+        let config = Config::test_with_forge_repo(forge_repo.clone());

@@ -1332,22 +1287,7 @@
+        let config = Config::test_with_forge_repo(forge_repo.clone());

@@ -1531,22 +1471,7 @@
+        let config = Config::test_with_forge_repo(forge_repo.clone());

@@ -1681,22 +1606,7 @@
+        let config = Config::test_with_forge_repo(forge_repo.clone());

@@ -1835,22 +1745,7 @@
+        let config = Config::test_with_forge_repo(forge_repo.clone());

Every test function in ci.rs that previously constructed a full Config struct literal now calls Config::test_with_forge_repo(forge_repo.clone()). The ForgeRepoConfig value was already built just above each call site, so the replacement is a clean substitution with no behavioral change.

This removes ~105 lines of duplicated config scaffolding from this single file.

Replace inline Config literals in mirror tests

Intent: Apply the same deduplication to the `base_config` helper in `mirror.rs`, which was the sole config factory for mirror-related tests.

Affected files: crates/pika-git/src/mirror.rs

Evidence

@@ -229,32 +229,17 @@
     fn base_config(canonical_git_dir: &str, mirror_remote: Option<&str>) -> Config {
-        Config {
-            repos: vec![...],
-            ...
-        }
+        Config::test_with_forge_repo(ForgeRepoConfig {
+            repo: "sledtools/pika".to_string(),
+            canonical_git_dir: canonical_git_dir.to_string(),
+            ...
+        })

The base_config() helper in mirror::tests now delegates to Config::test_with_forge_repo() and only specifies the ForgeRepoConfig fields that are meaningful to mirror tests (e.g., mirror_remote, mirror_poll_interval_secs). All other config fields come from defaults.

Replace inline Config literals in poller tests

Intent: Simplify the `config_for_bare_repo` helper in `poller.rs` using the same pattern.

Affected files: crates/pika-git/src/poller.rs

Evidence

@@ -149,32 +149,17 @@
     fn config_for_bare_repo(bare: &std::path::Path) -> Config {
-        Config {
-            repos: vec![...],
-            ...
-        }
+        Config::test_with_forge_repo(ForgeRepoConfig {
+            repo: "sledtools/pika".to_string(),
+            canonical_git_dir: bare.to_str().expect("bare path").to_string(),
+            ...
+        })

The config_for_bare_repo() helper in poller::tests now constructs only the ForgeRepoConfig (with mirror_remote: None and no mirror settings) and passes it to Config::test_with_forge_repo(). This removes the 15 boilerplate fields that were identical across every poller test.

Introduce web-module test helpers and replace inline Config literals

Intent: The web module has the most test functions and the most variation in config (some need admins, some don't, some override `ci_concurrency`). Add local helpers `forge_test_repo_config()`, `forge_test_config_without_admins()`, and refactor the existing `forge_test_config()` to use the new constructors, then replace all inline struct literals.

Affected files: crates/pika-git/src/web.rs

Evidence

@@ -3808,35 +3808,32 @@
-    fn forge_test_config() -> Config {
-        Config {
-            repos: vec![...],
-            ...
-        }
-    }
+    fn forge_test_repo_config(canonical_git_dir: impl Into<String>) -> ForgeRepoConfig {
+        ForgeRepoConfig {
+            repo: "sledtools/pika".to_string(),
+            canonical_git_dir: canonical_git_dir.into(),
+            ...
+        }
+    }
+
+    fn forge_test_config() -> Config {
+        let mut config = Config::test_with_forge_repo(forge_test_repo_config("/tmp/pika.git"));
+        config.bootstrap_admin_npubs = vec![TRUSTED_NPUB.to_string()];
+        config
+    }
+
+    fn forge_test_config_without_admins() -> Config {
+        let mut config = forge_test_config();
+        config.bootstrap_admin_npubs.clear();
+        config
+    }

@@ -3972,32 +3969,10 @@
-        let config = Config {
-            ...
-        };
+        let mut config = forge_test_config_with_git_dir(&root.path().join("pika.git"));
+        config.forge_repo.as_mut().expect("forge repo").hook_url =
+            Some("http://127.0.0.1:8788/git/webhook".to_string());
+        config.bootstrap_admin_npubs.clear();

@@ -4634,32 +4587,7 @@
-        let config = Config {
-            ...
-        };
+        let config = forge_test_config_without_admins();

@@ -5442,32 +5250,7 @@
+        let config = forge_test_config();

The web module's test infrastructure is restructured into three layers:

forge_test_repo_config(canonical_git_dir) — builds just the ForgeRepoConfig with sensible defaults; accepts any impl Into<String> for the git dir path.
forge_test_config() — calls Config::test_with_forge_repo(forge_test_repo_config(...)) and adds TRUSTED_NPUB to bootstrap_admin_npubs. This is the "admin-enabled" config.
forge_test_config_without_admins() — clears the admin list, used by tests that intentionally verify unauthenticated or non-admin behavior.

Tests that need to override a single field (e.g., ci_concurrency = None or a custom hook_url) do so via config.forge_repo.as_mut().expect("forge repo").field = ... after calling the helper. This is a clear pattern: start from a helper, mutate only what the test cares about.

The net removal in web.rs alone is approximately 400 lines of duplicated struct literals across ~18 test functions.

Replace inline Config literal in worker tests

Intent: Apply the same pattern to the single test in `worker.rs` that constructed a Config without a forge repo.

Affected files: crates/pika-git/src/worker.rs

Evidence

@@ -299,22 +299,8 @@
-        let config = Config {
-            repos: vec!["sledtools/pika".to_string()],
-            forge_repo: None,
-            ...
-            bootstrap_admin_npubs: vec![admin_npub.clone()],
-        };
+        let mut config = Config::test_defaults();
+        config.bootstrap_admin_npubs = vec![admin_npub.clone()];

The worker test is the only call site that needs a Config without any forge repo. It now uses Config::test_defaults() and overrides only bootstrap_admin_npubs. This is the intended use of the no-arg constructor—tests that exercise non-forge code paths.

pika-git-test-cleanup-3

CI: success

Summary

Tutorial Steps

Remove test-only fields from the Config struct

Add test-only constructor methods to Config

Replace inline Config literals in CI tests

Replace inline Config literals in mirror tests

Replace inline Config literals in poller tests

Introduce web-module test helpers and replace inline Config literals

Replace inline Config literal in worker tests

Diff