Skip to content

Add sandbox-based parallel test class for git_remote.resolve_sha #162

@cboos

Description

@cboos

Background

test/test_commit_linkifier.py::TestIntegrationLocalRepo validates the SHA resolver against the project's own git repo using 7c2e6f6 as a known historical SHA. This catches real-world things a mock couldn't — git CLI output format changes, GitHub URL canonicalization shifts, --contains behaviour — and is worth keeping for primary-CI smoke value.

But it has two gaps:

  1. Skips on forks. The @pytest.mark.skipif(not _project_repo_origin_is_daaain_repo(), …) predicate means contributors with a forked remote get zero coverage of the resolver.
  2. Brittle to repo evolution. The class needs fetch-depth: 0 in CI to reach historical SHAs (added in Linkify commit SHAs in rendered Markdown + HTML — closes #156 #161 as commit b3572b0). If main is ever force-pushed past 7c2e6f6, or the remote moves, the test breaks.

Proposal

Add a parallel TestSandboxResolver class that runs everywhere (no skip), alongside the existing TestIntegrationLocalRepo. Setup per test method:

  1. tempfile.TemporaryDirectory()git init a fresh repo
  2. Configure a fake remote: git remote add origin git@github.com:owner/x.git (or HTTPS variant for URL-parser coverage)
  3. Make 1–2 commits with deterministic content; capture the SHA
  4. Manually create the remote-tracking ref: git update-ref refs/remotes/origin/main <sha> (avoids needing a real git fetch)
  5. Call resolve_sha(tmpdir, short_sha) and assert URL shape

This exercises the resolver's full subprocess pipeline (git config, git branch -r --contains, git rev-parse --verify) without touching the network or depending on the project's own history.

Why not replace TestIntegrationLocalRepo?

The local-repo tests catch a different class of bug — they cross-check that the same code that works in a fabricated fixture also works against an organically-grown git repo with real refs, real remote URL, real history depth. Both layers are valuable: hermetic + smoke-against-reality.

Acceptance criteria

  • New TestSandboxResolver class with at least: resolve happy-path, unresolved-SHA returns None, short-SHA expansion, URL-template selection (GitHub host), unknown-host returns None
  • Runs without skip on forks (no @pytest.mark.skipif)
  • No new external deps; uses stdlib tempfile + subprocess
  • CI still passes with both TestIntegrationLocalRepo and TestSandboxResolver running

Out of scope (for this issue)

  • Replacing or shrinking TestIntegrationLocalRepo — keep it as the reality-check layer.
  • Reverting fetch-depth: 0 in .github/workflows/ci.yml. Once the sandbox class exists, that becomes optional, but flipping it should be a separate decision once we see how often the local-repo class catches things the sandbox doesn't.

Context: discussion on PR #161 review.

Metadata

Metadata

Assignees

No one assigned

    Labels

    cleanupCode and test clean-ups

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions