test(git-fetcher): drop skip_if_no_{git,npm} guards

claude · claude · commit 890dd72006a5 · 2026-05-18T08:25:06.000Z
Replace runtime probe-and-skip helpers with the existing `.unwrap()`
calls — if `git`, `node`, or `npm` is missing the test will fail
loudly rather than silently return. Tolerance defeats the purpose of
testing: an under-provisioned environment is a bug in the environment,
not something the suite should paper over. Git is ubiquitous and
Node.js is a documented prerequisite for building pnpm.

Codify the rule in pacquet/AGENTS.md so future ports don't reintroduce
the pattern. Platform-locked tools are still allowed to gate via
`#[cfg(...)]` or `#[cfg_attr(..., ignore = "...")]`, which surface in
the test report instead of disappearing.

---
Written by an agent (Claude Code, claude-opus-4-7).
diff --git a/pacquet/AGENTS.md b/pacquet/AGENTS.md
@@ -204,6 +204,48 @@ Warnings are errors (`--deny warnings` in lint). Do not silence them with
   regression. Consult it before adding ported tests, and update its
   checkboxes as items land.
 
+### No "tolerant" tests for missing tools
+
+Tests must not be tolerant of a missing build / runtime environment by
+silently `return`-ing early when a tool isn't found. Patterns like:
+
+```rust
+fn skip_if_no_git() -> bool {
+    if std::process::Command::new("git").arg("--version").output().is_err() {
+        eprintln!("skipping: `git` not on PATH");
+        return true;
+    }
+    false
+}
+
+#[test]
+fn my_test() {
+    if skip_if_no_git() {
+        return;
+    }
+    // ...
+}
+```
+
+are forbidden. If the test needs a tool, just call into it and let the
+existing `.unwrap()` / `.expect(...)` panic when the tool is absent — a
+failing test in an under-provisioned environment is the correct signal.
+Tolerance defeats the purpose of testing: if the environment really
+doesn't have the required tools, that's the *environment's* fault and it
+needs to be fixed.
+
+This applies in particular to `git`, `node`, and `npm` — git is ubiquitous
+on developer machines, and Node.js is a documented prerequisite for
+building pnpm. There is no realistic environment in which pacquet's tests
+should run *and* these tools should be absent.
+
+The only marginally acceptable exception is platform-locked tools — APIs
+or binaries that exist on one OS but not another. Even then, prefer
+`#[cfg_attr(target_os = "windows", ignore = "...")]` (or the matching
+`#[cfg(unix)]` gate already used in this crate for `/bin/sh` shims) over a
+runtime probe-and-skip helper. The gate is visible to `cargo test` and
+shows up in the test report; a silent `return` does not.
+
 ### Running tests narrowly
 
 Running the full suite is slow. While iterating, target what you're working
diff --git a/pacquet/crates/git-fetcher/src/fetcher/tests.rs b/pacquet/crates/git-fetcher/src/fetcher/tests.rs
@@ -8,33 +8,6 @@ use pacquet_testing_utils::env_guard::EnvGuard;
 use std::{fs, path::Path, path::PathBuf};
 use tempfile::tempdir;
 
-fn skip_if_no_git() -> bool {
-    let probe = std::process::Command::new("git").arg("--version").output();
-    if probe.is_err() {
-        eprintln!("skipping: `git` not on PATH");
-        return true;
-    }
-    false
-}
-
-/// Skip the test if `npm` (and `node`) aren't on `PATH`. Used by the
-/// real-prepare tests below, which spawn an actual lifecycle script:
-/// `prepare_package` synthesizes `npm install` for any non-pnpm
-/// preferred-pm, and that needs an npm binary to invoke. Hosts
-/// without node tooling (a Rust-only sandbox, an embedded CI image)
-/// silently skip rather than fail so the suite stays portable.
-fn skip_if_no_npm() -> bool {
-    if std::process::Command::new("npm").arg("--version").output().is_err() {
-        eprintln!("skipping: `npm` not on PATH");
-        return true;
-    }
-    if std::process::Command::new("node").arg("--version").output().is_err() {
-        eprintln!("skipping: `node` not on PATH");
-        return true;
-    }
-    false
-}
-
 /// Build a bare repo whose manifest declares a `prepare` script. The
 /// script is whatever the caller passes — typically a `node -e '…'`
 /// one-liner that writes a marker file or exits non-zero. Returns
@@ -122,9 +95,6 @@ fn extract_host_handles_user_authority_and_port() {
 
 #[tokio::test(flavor = "multi_thread")]
 async fn fetcher_imports_package_into_cas() {
-    if skip_if_no_git() {
-        return;
-    }
     let tmp = tempdir().unwrap();
     let (bare, commit) = make_bare_repo(tmp.path());
     let store_root = tempdir().unwrap();
@@ -164,9 +134,6 @@ async fn fetcher_imports_package_into_cas() {
 
 #[tokio::test(flavor = "multi_thread")]
 async fn fetcher_rejects_commit_mismatch() {
-    if skip_if_no_git() {
-        return;
-    }
     let tmp = tempdir().unwrap();
     let (bare, _commit) = make_bare_repo(tmp.path());
     let store_root = tempdir().unwrap();
@@ -211,9 +178,6 @@ async fn fetcher_rejects_commit_mismatch() {
 
 #[tokio::test(flavor = "multi_thread")]
 async fn fetcher_blocks_build_when_not_allowed() {
-    if skip_if_no_git() {
-        return;
-    }
     let tmp = tempdir().unwrap();
     // A repo whose manifest declares a `prepare` script — exercises
     // the `allow_build` gate without actually spawning the script
@@ -337,9 +301,6 @@ fn make_bare_repo_without_manifest(tmp: &Path) -> (PathBuf, String) {
 /// the monorepo root or sibling packages.
 #[tokio::test(flavor = "multi_thread")]
 async fn fetcher_packs_subfolder_when_path_set() {
-    if skip_if_no_git() {
-        return;
-    }
     let tmp = tempdir().unwrap();
     let (bare, commit) = make_monorepo_bare_repo(tmp.path());
     let store_root = tempdir().unwrap();
@@ -387,9 +348,6 @@ async fn fetcher_packs_subfolder_when_path_set() {
 /// downstream, but the fetcher itself must not crash.
 #[tokio::test(flavor = "multi_thread")]
 async fn fetcher_handles_repo_without_package_json() {
-    if skip_if_no_git() {
-        return;
-    }
     let tmp = tempdir().unwrap();
     let (bare, commit) = make_bare_repo_without_manifest(tmp.path());
     let store_root = tempdir().unwrap();
@@ -433,9 +391,6 @@ async fn fetcher_handles_repo_without_package_json() {
 /// build (matches upstream's `shouldBeBuilt = true` short-circuit).
 #[tokio::test(flavor = "multi_thread")]
 async fn fetcher_skips_build_when_ignore_scripts() {
-    if skip_if_no_git() {
-        return;
-    }
     let tmp = tempdir().unwrap();
     // A repo whose `prepare` script would fail if it ran — observing
     // success proves the lifecycle runner never spawned anything.
@@ -510,9 +465,6 @@ async fn fetcher_skips_build_when_ignore_scripts() {
 /// actually executed (the file didn't exist in the source tree).
 #[tokio::test(flavor = "multi_thread")]
 async fn fetcher_runs_prepare_script_when_allowed() {
-    if skip_if_no_git() || skip_if_no_npm() {
-        return;
-    }
     let tmp = tempdir().unwrap();
     // The prepare script writes a marker. Single-quoted inner
     // string so the JSON doesn't need to escape it; node reads the
@@ -569,9 +521,6 @@ async fn fetcher_runs_prepare_script_when_allowed() {
 /// fetcher refuses to add a broken-build snapshot to the CAS.
 #[tokio::test(flavor = "multi_thread")]
 async fn fetcher_surfaces_prepare_failure() {
-    if skip_if_no_git() || skip_if_no_npm() {
-        return;
-    }
     let tmp = tempdir().unwrap();
     let (bare, commit) = make_bare_repo_with_prepare_script(
         tmp.path(),
@@ -641,9 +590,6 @@ async fn fetcher_surfaces_prepare_failure() {
 /// keep the block-test green.
 #[tokio::test(flavor = "multi_thread")]
 async fn fetcher_runs_prepare_when_allow_build_returns_true() {
-    if skip_if_no_git() || skip_if_no_npm() {
-        return;
-    }
     let tmp = tempdir().unwrap();
     let (bare, commit) = make_bare_repo_with_prepare_script(
         tmp.path(),
