Repository navigation
pre-commit registers only the commit's own tests - #22056
Conversation
Red: committing tests/test_mine.py also registers the base's unregistered tests/test_base.py, so every pull request that commits a test carries the same registry entries and they conflict (#21946); register_derivable has no way to name the tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The hook ran the registry writer over every unregistered test, so whichever pull request committed a test next also carried the base's stragglers, and those identical entries conflicted across pull requests. register_derivable takes --only; the hook passes the tests the commit adds and the ones its workflow changes name. The base's two stragglers are registered here once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Bead |
This comment has been minimized.
This comment has been minimized.
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Unclear: CI failed on
Not re-run automatically: Written by red: web / Web status; do not merge until green; see lane-rules
Agents: |
…s-own-files-only # Conflicts: # tests/test-execution.toml
[Medium impact] The PR appears safe to merge, with a non-blocking filename mismatch in workflow-only registration. Findings
SummaryLimits automatic registration to tests added by the commit or named on added workflow lines.
The previous and current commits have different merge bases with the supplied target, so this review used the full PR diff. Tests were not run. Reviews (3) · Last reviewed commit: "Merge remote-tracking branch 'origin/fea..." · Reviewed by Greptile |
Greptile on #22056: the hook test added the test file, so the path that collects tests from added workflow lines went untested. Fails on the old hook (it also registers the base's unregistered test). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
| done < <( | ||
| { | ||
| git diff --cached --name-only --diff-filter=A -- 'tests/test_*.py' | ||
| git diff --cached -U0 -- .github/workflows | grep -E '^\+' | grep -oE 'tests/test_[A-Za-z0-9_]+\.py' || true |
There was a problem hiding this comment.
The workflow matcher excludes supported test names containing - or .. If a commit adds a workflow line for an existing unregistered tests/test_foo-bar.py, the hook finds no path and leaves it unregistered. The old hook registered these tests; contributors now need to do that manually.
Use the validator's [A-Za-z0-9_.-]+ filename pattern here too.
| git diff --cached -U0 -- .github/workflows | grep -E '^\+' | grep -oE 'tests/test_[A-Za-z0-9_]+\.py' || true | |
| git diff --cached -U0 -- .github/workflows | grep -E '^\+' | grep -oE 'tests/test_[A-Za-z0-9_.-]+\.py' || true |
|
Shipped in:
|
The pre-commit hook registers only the commit's own tests: the ones it adds, and the ones its workflow changes name. Part of #21946.
Before, the hook ran the registry writer over every unregistered test in the repository. If the base had a straggler, whichever pull request committed a test next carried it too. Every PR then added the same registry entries, and they conflicted with each other. The click-through lane hit this on #21272 with
tests/test_cmux_next_base_autofix_finishes.pyandtests/test_nightly_next_feed_rollback.py. This PR registers those two once on the base.Change
validate_test_execution_registry.py --writetakes a repeatable--only PATH, andregister_derivable(root, only=...)registers just those paths. With no--only, it registers everything, as before.git diff --cached:tests/test_*.pypaths on added lines in workflow files.Tests (committed failing first)
tests/test_install_git_hooks.py,test_pre_commit_registers_only_the_commits_own_tests. A repository whose base has an unregisteredtest_base.pycommitstest_mine.py.test_mine.py.tests/test_ci_test_execution_registry.py,test_write_only_registers_the_named_tests.TypeError, sinceonlydidn't exist.test_install_git_hookspasses.LaneRunnerTests.test_verify_local_jobs_option_keeps_recipe_tests_livefails both before and after this change in this sparse checkout.The installed hook copies refresh when
scripts/install-git-hooks.shreruns, whichpost-mergedoes. A clone that still has the old validator copy ignores--onlywith an error, and the hook continues because registration is best effort.Changelog
none
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Stops the pre-commit hook from writing the base's unregistered tests into every pull request. Part of #21946.
Before, the hook ran the registry writer over every unregistered test in the repository, so a stray test on the base got carried into whichever PR committed a test next, and identical registry entries conflicted across PRs. Now the hook registers only the tests the commit adds or that its workflow changes name. The base's two stragglers (
tests/test_cmux_next_base_autofix_finishes.py,tests/test_nightly_next_feed_rollback.py) are registered once here.Changes
validate_test_execution_registry.py --writeaccepts a repeatable--only PATH; without it, behavior is unchanged.git diff --cached: addedtests/test_*.pyfiles and test paths on added lines in workflow files.Written for commit e571cad. Summary will update on new commits.