Skip to content

bench: share one source_sha() across the harnesses (#1387) - #1403

Merged
MakazhanAlpamys merged 2 commits into
MakazhanAlpamys:mainfrom
maximilianfeix:bench/1387-shared-source-sha
Sep 29, 2026
Merged

MakazhanAlpamys merged 2 commits into
MakazhanAlpamys:mainfrom
maximilianfeix:bench/1387-shared-source-sha

Conversation

@maximilianfeix

Copy link
Copy Markdown

What does this PR do?

  • benchmarks/harness/source_provenance.py has the one source_sha(). variant2_gate, issue361_nf4_throughput and head_prefetch_ab keep their _source_sha() name (the harness tests and _versions() use it) but it just calls the shared one. head_prefetch_ab keeps its -- src dirty scope through a dirty_pathspec argument, the other two count every change as before.
  • New check: after resolving the checkout, git ls-files --error-unmatch on soup_cli/__init__.py. A wheel installed into a gitignored .venv/ inside the checkout now records unknown instead of the checkout's commit.
  • tests/test_issue1387_source_provenance.py runs real git against a throwaway checkout with a stand-in soup_cli: clean → commit, tracked edit → -dirty, pathspec scoping, untracked copy inside the checkout → unknown, git archive tree → unknown, and each harness delegating (with the right pathspec). The CalledProcessError and 40-hex cases were already pinned in test_issue1174_* and still pass.
  • The three existing provenance tests now expect the extra ls-files call, and patch subprocess.run directly since the harnesses no longer import subprocess. test_issue361's loader puts benchmarks/harness on sys.path like the other two, since the harness now imports its sibling (as it does when run as a script).

Type of change

  • Refactor / cleanup
  • Tests

Checklist

  • ruff check src/soup_cli/ scripts/ tests/ passes
  • pytest tests/ -v passes
  • Updated relevant docs (README.md and the matching page under docs/) if needed
  • Added a changelog fragment, or this change has no user-visible impact

Ran the four provenance test files (82 passed), each one on its own too so the sys.path handling doesn't depend on test order; not the full suite.

Closes #1387

@MakazhanAlpamys MakazhanAlpamys left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maximilianfeix, thank you, and welcome. This is a careful first pass at a fiddly piece of code, and the design is right in the two places it most needed to be:

  • head_prefetch_ab keeps its -- src dirty scope through dirty_pathspec, while variant2_gate and issue361_nf4_throughput keep counting every change. Nobody inherits the narrowest scope.
  • The new tests run real git against a throwaway checkout instead of faking subprocess.run, and that choice is what pins the fix. Of the mutations below, swallowing an ls-files failure is caught by exactly one test, test_untracked_copy_inside_the_checkout_is_unknown. None of the fake-based tests could have seen it.

Smaller things I liked:

  • The _issue1387_ prefix in _load() keeps your module objects from replacing the ones the other three test files register.
  • Keeping the _source_sha() names means _versions() and the existing monkeypatches keep working.
  • The three call-order tests now pin the ls-files argv and cwd as well.

What I ran (Windows, Python 3.12, on 8cc009d2)

  • The four test files: 82 passed. Each file alone: 8 / 8 / 33 / 33. In reverse file order: 82. After merging current main into the branch: 82 passed, and ruff check is clean.
  • A before/after comparison with real git: three harnesses × 13 checkouts, main's harness files against yours. HEAD means the checkout's own commit.
Checkout variant2_gate issue361_nf4_throughput head_prefetch_ab
clean HEAD HEAD HEAD
untracked file at repo root (an --out file) HEAD-dirty HEAD-dirty HEAD
tracked edit outside src/ HEAD-dirty HEAD-dirty HEAD
tracked edit in src/ HEAD-dirty HEAD-dirty HEAD-dirty
untracked new file in src/ HEAD-dirty HEAD-dirty HEAD-dirty
wheel in gitignored .venv/, clean checkout HEAD → unknown HEAD → unknown HEAD → unknown
wheel in gitignored .venv/, tracked edit in src/ HEAD-dirty → unknown HEAD-dirty → unknown HEAD-dirty → unknown
wheel in an untracked, not-ignored vendor/ HEAD-dirty → unknown HEAD-dirty → unknown HEAD → unknown
soup_cli outside any checkout unknown unknown unknown
git archive extract unknown unknown unknown
checkout reached through a symlink HEAD HEAD HEAD
checkout folder named co [1] x HEAD HEAD HEAD
the real working tree I ran from HEAD HEAD HEAD

Only the three wheel rows change, and they change to unknown. A checkout under a non-ASCII path (Müller é checkout) also resolves to HEAD. That is worth knowing now that the package path goes to git as an argument, not only as a cwd.

  • Mutations. Each one re-ran all four files (82 tests) and was then reverted:
    • drop the ls-files call (source_provenance.py:49): 7 fail
    • swallow an ls-files failure (same line): 1 fails
    • head_prefetch_ab._source_sha without dirty_pathspec="src" (head_prefetch_ab.py:126): 3 fail
    • the helper ignores dirty_pathspec (source_provenance.py:52): 3 fail
    • variant2_gate._source_sha stops delegating (variant2_gate.py:215): 3 fail
    • 40-hex guard removed: 1 fails; except OSError only: 3 fail; dirty suffix inverted: 9 fail
  • On main, without your change:
    • The new file cannot import, since there is no helper yet.
    • Your three updated call-order tests fail 6 / 68.
    • With main's logic dropped in under the helper's name, the new file goes 4 failed / 4 passed. The wheel case returns the checkout's clean SHA instead of unknown, and the three delegation checks fail. The clean, dirty, pathspec and archive tests pass, as behaviour-preserving tests should.

One small change, all in the new test's fixture

  1. tests/test_issue1387_source_provenance.py:47: the "git is not installed" skip can't fire. When there is no git executable, subprocess.run(["git", "--version"]) raises FileNotFoundError instead of returning a non-zero code. The five real-git tests therefore error instead of skipping. With git removed from PATH I got 3 passed, 5 errors, 0 skipped. shutil.which("git") is None fixes it: 3 passed, 5 skipped.
  2. tests/test_issue1387_source_provenance.py:55: the fixture's commit inherits the developer's global git config. Some developers set commit.gpgsign=true, which is common for anyone who signs their commits. I simulated that with a throwaway GIT_CONFIG_GLOBAL: git commit fails and the same five tests error (3 passed, 5 errors). tests/test_issue1017_merge_freshness.py:81 already handles this with -c commit.gpgsign=false.
  3. Optional, same paste: :99-101 needs tar as a second external tool. On Windows, subprocess finds C:\Windows\System32\tar.exe before anything on PATH. Here it ran bsdtar 3.8.8, while shutil.which("tar") reports Git's tar. It works, but git archive --format=zip plus zipfile avoids the question entirely.

Ready to paste. It is ruff-clean. With it, the file gives:

  • 8 passed normally
  • 3 passed and 5 skipped without git
  • 8 passed with signing required

The eight mutations above are killed with the same counts as before.

--- a/tests/test_issue1387_source_provenance.py
+++ b/tests/test_issue1387_source_provenance.py
@@ -8,9 +8,12 @@ package, so the resolution is exercised end to end rather than through a fake
 from __future__ import annotations
 
 import importlib.util
+import io
+import shutil
 import subprocess
 import sys
 import types
+import zipfile
 from pathlib import Path
 
 import pytest
@@ -44,7 +47,7 @@ def _git(cwd: Path, *args: str) -> str:
 
 @pytest.fixture
 def checkout(tmp_path: Path) -> Path:
-    if subprocess.run(["git", "--version"], capture_output=True).returncode != 0:
+    if shutil.which("git") is None:
         pytest.skip("git is not installed")
     root = tmp_path / "checkout"
     (root / "src" / "soup_cli").mkdir(parents=True)
@@ -52,7 +55,11 @@ def checkout(tmp_path: Path) -> Path:
     (root / ".gitignore").write_text(".venv/\n", encoding="utf-8")
     _git(root, "init", "-q")
     _git(root, "add", ".")
-    _git(root, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", "init")
+    _git(
+        root,
+        "-c", "user.name=t", "-c", "user.email=t@t", "-c", "commit.gpgsign=false",
+        "commit", "-q", "-m", "init",
+    )
     return root
 
 
@@ -96,9 +103,9 @@ def test_git_archive_tree_is_unknown(monkeypatch, checkout, tmp_path):
     extracted = tmp_path / "extracted"
     extracted.mkdir()
     archive = subprocess.run(
-        ["git", "archive", "HEAD"], cwd=checkout, check=True, capture_output=True
+        ["git", "archive", "--format=zip", "HEAD"], cwd=checkout, check=True, capture_output=True
     ).stdout
-    subprocess.run(["tar", "-x", "-C", str(extracted)], input=archive, check=True)
+    zipfile.ZipFile(io.BytesIO(archive)).extractall(extracted)
     _import_soup_cli_from(monkeypatch, extracted / "src" / "soup_cli" / "__init__.py")
     assert provenance.source_sha() == "unknown"

No changelog fragment is needed. This change is benchmarks and tests only, the same as #1382, which last changed this function. No docs change is needed either. The head_prefetch_ab row in benchmarks/README.md says the JSON records "the tree it measured", and after this change that is more accurate than before.

That is everything I have for this PR. The required checks on 8cc009d2 are still queued, and the next push will get its own run.

@maximilianfeix

Copy link
Copy Markdown
Author

Thanks, and thanks for the before/after table, the non-ASCII path check is good to know. Applied your diff as is. Here: 8 passed normally, 3 passed / 5 skipped with git off PATH, and 8 passed with a throwaway GIT_CONFIG_GLOBAL that sets commit.gpgsign = true. ruff clean.

@MakazhanAlpamys MakazhanAlpamys left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maximilianfeix, thanks for the quick turnaround. I checked the new head, bea91d28:

  • The only change since my review is in tests/test_issue1387_source_provenance.py (+11/-4), and it is exactly the three fixture fixes.
  • The four provenance test files: 82 passed.
  • With git removed from PATH: 77 passed, 5 skipped. All five skips say "git is not installed"; before this push they errored.
  • With a throwaway GIT_CONFIG_GLOBAL that sets commit.gpgsign = true: 82 passed.
  • Swallowing an ls-files failure (source_provenance.py:49) is still caught by test_untracked_copy_inside_the_checkout_is_unknown. The other seven mutations from my first review are caught with the same counts.
  • Merged with current main: no conflicts, 82 passed, ruff check clean.

That matches the numbers in your comment. From my side this is ready to merge. It now waits only on the required checks, which are queued on bea91d28.

@MakazhanAlpamys MakazhanAlpamys added the ci:full Run the full CI matrix on this PR (a maintainer adds it at approval) label Sep 28, 2026
@MakazhanAlpamys
MakazhanAlpamys merged commit 5df5b8d into MakazhanAlpamys:main Sep 29, 2026
19 checks passed
@MakazhanAlpamys

Copy link
Copy Markdown
Owner

Merged as 5df5b8d5. Thank you, @maximilianfeix: you tested the fix with real git against a throwaway checkout instead of a faked subprocess.run. That choice is the only reason a swallowed ls-files failure gets caught at all. Here, the four provenance test files gave 82 passed, on the branch and merged with main. They gave 77 passed / 5 skipped with git off PATH, and 82 passed with commit signing required. All 8 mutations of the helper and the harnesses are caught (7, 3, 3, 1, 3, 1, 3 and 9 failures).

MakazhanAlpamys added a commit that referenced this pull request Sep 30, 2026
…1370, #1371, #1372, #1375, #1379, #1386, #1388, #1397, #1400, #1402, #1403, #1404, #1410, #1108, #1143, #1133

Display names from `gh api users/<login>`.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:full Run the full CI matrix on this PR (a maintainer adds it at approval)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Benchmark harnesses: share one _source_sha helper, and do not credit the checkout for a wheel installed inside it

2 participants