Repository navigation
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change makes the ChangesServices package import behaviour
Pre-push pytest selection
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Git
participant prepush
participant prepush_selection
participant Pytest
Git->>prepush: Provide changed paths
prepush->>prepush_selection: Select applicable tests
prepush_selection-->>prepush: Return tests and selection reasons
prepush->>Pytest: Run selected tests with scaled timeout
Pytest-->>prepush: Return test result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation Issue Full details: Docstring CoverageExplanation Docstring coverage is 73.68% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 5 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
…tes, so Package.setup doesn't read an invented pytest_plugins (#16722) CI at 9251f0d failed shards 5 and 7 with 59 setup errors across the services/ tests, all "pytest.UsageError: Plugins may be specified as a sequence ... Got: <MagicMock name='mock.pytest_plugins'>". The cause: now that the stub is kept, Package.setup imports services/__init__.py through importtestmodule, and importlib mode returns the stub already in sys.modules. pytest then reads pytest_plugins (via consider_module) and setUpModule, setup_module, tearDownModule and teardown_module off it, and MagicMock invents all five. The stub now carries what a plain module without them gives: [] and None. The regression test drives importtestmodule itself.
|
Root cause of the python-suite shard 5 and shard 7 reds at Shards 5 and 7 hit 59 setup errors across the
The mechanism, from pytest 9.1.1's source:
Before this PR, pytest had already swapped the real package over the stub, so this path never saw a MagicMock. The fix. The stub now carries exactly those five attributes, with the values a plain module that lacks them would give: Neither failing shard reported a sys.modules leak failure, which supports the Pre-push at |
|
Review at 52a3638: approve. Full PR (all three commits — #16711, #16722, and the regression fix on top). #16711, `tools/lint/prepush_selection.py` — clean, testable extraction of what was hand-rolled shell (`existing_py_files`, `pytest_ignored`, the co-located-sibling loop). `pytest_settings()` reads `python_files` from `pytest.ini` directly rather than hardcoding `*_test.py`, fixing the actual #16700 incident (a changed `tests/test_x.py` wasn't recognized as a test at all). Confirmed the module resolves (`tools/init.py` and `tools/lint/init.py` both exist) and #16715 is a real, open, related issue. `test_the_16700_changeset_runs_every_test_it_changed` pins the regression to the actual incident rather than a synthetic case. Pre-push hook integration — a failed selection now blocks the push outright (`EXIT_CODE=1`, empty selection) rather than silently reading as "nothing to verify," which is the right fail-closed direction for a tool whose whole job is deciding what gets verified. #16722, the `path` fix — matches the described `_pytest.pathlib._import_module_using_spec` mechanism: a path-less MagicMock stub gets re-imported from disk by pytest's importlib mode on first submodule collection, and the leak baseline's removed `a2a_card_fetcher_test.py` entry checks out as a genuine ratchet shrink (the swap that entry existed for no longer happens). The regression fix (52a3638) — the part I hadn't seen. Once the stub survives via `path`, `Package.setup` reads `pytest_plugins`/xunit hooks off it and MagicMock invents all five, one of which (`pytest_plugins`) is read as if it were real config and throws `UsageError`. Giving the stub `pytest_plugins = []` and the four hooks `None` is exactly what a plain module without them would present. `test_pytest_sets_up_the_services_package_with_the_stub_in_place` drives `importtestmodule` directly — the real pytest internal, not a reimplementation. Best part of the test suite: `test_pytest_keeps_a_package_stub_that_has_a_path` / `test_pytest_replaces_a_package_stub_without_a_path` pin the mechanism against real `_pytest.pathlib.import_path` on a scratch package, with the second one explicitly framed as a canary ("if pytest stops doing this, the fix above is no longer needed") — the fix's premise is falsifiable, not assumed. CI: no FAILURE/ERROR/TIMED_OUT checks. Mergeable. |
…k with a __path__ (#16722) Code review of 52a3638 (HIGH): a MagicMock given a __path__ still invents every attribute. So `from services import manifest_loader`, `jwks_verifier`, `frontend_bundle_health` and the rest bound invented mocks instead of importing the real submodule. And tests/services/conftest.py's _ensure_real_pkg guard now short-circuited on that __path__, so the hollow-package swap it used to do never happened. The package-level UsageError in the last red run hid those tests. The root conftest now installs a hollow types.ModuleType("services") over the real directory, the shape tests/services/conftest.py already uses, and binds each child stub onto it. It invents nothing, so pytest's Package.setup attribute reads and `from services import` both behave as they would on a real package, and the explicit pytest_plugins and xunit-hook attributes are no longer needed. The regression test checks the shape instead of MagicMock identity: tests/services modules install their own hollow package at import time. It also checks that `from services import purge_playbook`, an unstubbed submodule, loads the real file.
|
My code review of
The package-level The fix:
Pre-push at |
|
Delta review at a5ed57b (from 52a3638): approve. Verified the HIGH is real, not just plausible: Python's `from pkg import name` first tries `getattr(pkg, name)` before attempting `import pkg.name` from disk. A MagicMock's catch-all `getattr` never raises `AttributeError`, so that first check always "succeeds" with an invented mock — the real-file import is never attempted for any submodule not explicitly bound as a child stub. Adding `path` to the MagicMock (52a3638) fixed the package-detection problem but did nothing about this, since it's a completely different code path. Confirmed the fix closes it: `from services import purge_playbook` — genuinely unstubbed (not in `conftest.py`'s stub lists, confirmed by grep, and the file exists on disk) — is exactly the right known-positive, and a plain `types.ModuleType` raises real `AttributeError` for it, correctly falling through to the real import. Confirmed the second half too: `tests/services/conftest.py`'s `_ensure_real_pkg` guard is `if existing is not None and hasattr(existing, "path"): return` — the old MagicMock-with-`path` satisfied this and skipped the hollow-package install that guard exists to perform. The new root-conftest hollow package satisfies the SAME guard for the right reason ("already a package (real or hollow)"), so the two conftests now cooperate instead of one masking the other's work. Test suite adapted well: shape checks replace identity checks (matches the doc's own point — `tests/services/` installs its own hollow package at import time, so asserting `is` the root conftest's object would be testing the wrong thing), the path comparison now resolves both sides before comparing, and the pytest-keep-vs-replace canary tests were updated to use a hollow `ModuleType` instead of a MagicMock, still pinned against real `_pytest.pathlib.import_path`. CI: no FAILURE/ERROR/TIMED_OUT checks. Mergeable. |
…gisters onto the parent, so the two never diverge (#16722)
|
Root cause of the python-suite shard 12 red at The failure was Mechanism. At collection time, Why it showed up now. On main the same sequence happens whenever these two files share a shard. The identity test's own docstring says the outcome depends on which test ran last in the shard. This PR adds a test file, which changes how pytest-split composes the shards, and that put the two files together in shard 12. The other tests that swap Fix. The lockstep test binds the module it registers onto the parent as well, so both views are the same object. Pre-push at |
|
Delta review at b3310df (from a5ed57b): approve. Verified against `test_real_service_modules_14307.py`'s own docstring for `test_the_parent_never_exposes_a_different_object_than_sys_modules`: it explicitly asserts "never disagrees" rather than "is always present," specifically to avoid false positives from the two conftests' legitimate stub-swap ordering — so this is a genuine #9780-class divergence bug, not a shard-composition false alarm. The fix is trivially correct: `setattr(sys.modules["services"], "deploy_artifacts", deploy_artifacts)` immediately after `sys.modules["services.deploy_artifacts"] = deploy_artifacts` uses the exact same object reference for both assignments, so `sys.modules["services.deploy_artifacts"] is getattr(sys.modules["services"], "deploy_artifacts")` holds by construction — closing exactly the gap the identity test checks for. CI: no FAILURE/ERROR/TIMED_OUT checks. Mergeable. |
|
Carried into vehicle #17119 (member head |
…'s selection call (#17008, #16711) Discovered pushing the vehicle-77 main-merge: this vehicle is the first time #16728's real `python3 -m tools.lint.prepush_selection` call (added to the hook's Backend/Python phase) and #17008's synthetic-repo sandbox test combine, and the sandbox was missing what that call needs -- mktemp/rm (the hook's own error-capture temp file), python3 itself, a PYTHONPATH pointing at the real repo root (cwd is the synthetic repo, which has no tools/ package of its own), and a minimal pytest.ini in the synthetic repo (prepush_selection.py reads cwd/pytest.ini directly). Nothing here changes what's under test; it lets the real selection tool run against the synthetic repo's git state instead of dying immediately.
1 participant
Closes #16711
Refs #16722, #16715, #16700
Two pre-push problems, fixed together at the owner's direction because both sit in the hook's test selection. #16722 blocks the push of #16020/#16712.
Thinking Path
--collect-only, before any test body runs, and a try/finally inside the test didn't help. Reading pytest 9.1.1's_pytest/pathlib.py:_import_module_using_specexplains it. In importlib mode, pytest imports a test module's parent package first, and re-imports that package from disk whenever the object insys.moduleshas no__path__. The slm conftest'sservicesstub is a bare MagicMock, so collecting any test module underautobot-slm-backend/services/ran the realservices/__init__.pyover the stub.services/test is collected first. In CI that's each shard's alphabetically first file, and the baseline already lists it. CI's own leak report on main (run 34811729670) showssys.modules['services'] was replaced by autobot-slm-backend/services/a2a_card_fetcher_test.pyas that owner's single known key. Pre-push's two-file selection madereconciler_*the first, and it isn't on the baseline. Reordering the hook can't fix this, becauseservices/already sorts first. The same order dependence is latent in CI: a shard rebalance would blame a new file.*_test.py, so a changedtests/test_*.pywas treated as production code and never ran.What Changed
autobot-slm-backend/conftest.py(sys.modules leak guard: reconciler_status_restart_delta_14465_test.py leaks 'services' into a later autobot-slm-backend/tests file #16722). Theservicesparent is now a hollowtypes.ModuleTypeover the real directory, the shapetests/services/conftest.pyalready uses, with each child stub bound onto it. It took three commits to get here, each a regression fixed on this PR:9251f0d91gave the MagicMock stub a__path__, so pytest stopped re-importing the real package. But pytest'sPackage.setupthen got the stub back fromsys.modulesand readpytest_pluginsoff it. The MagicMock invented one, causing 59UsageErrors (shards 5 and 7).52a3638cbset those five attributes explicitly. My code review then found that a MagicMock with__path__still invents attributes, sofrom services import <submodule>bound a mock. Also,tests/services/conftest.py's_ensure_real_pkgguard now short-circuited on the stub's__path__.a5ed57ba9replaced the MagicMock with a hollow package, which invents nothing.tests/services/test_deploy_artifacts_lockstep.py, commitb3310df4f. This test registered a freshservices.deploy_artifactsinsys.moduleswithout binding it onto the parent, a discovery(tests): conftest services.* MagicMock stub defeats patch('services.X.Y') target resolution #9780 divergence that was latent on main and depends on shard composition. This PR's new test file reshuffled the shards, putting it together withtest_real_service_modules_14307in shard 12. It now binds the module onto the parent too.repo_tests/sys_modules_leak_baseline.txt. Theautobot-slm-backend/services/a2a_card_fetcher_test.pyline is deleted: CI's report gives this exactservicesreplacement as its only key.reconciler_check_node_health_test.pystays, because its known key is a different one (a syntheticservices.compose_fleet).autobot-slm-backend/tests/test_services_stub_is_a_package_16722.py(new). It checks:servicesis aModuleTypeover the real directory, with nothing invented;from services import purge_playbookloads the real submodule;importtestmoduleonservices/__init__.pysucceeds;__path__, the mechanism itself.tools/lint/prepush_selection.py(new, ci(pre-push): a changed test file named test_*.py never runs in the pre-push hook #16711). It readspython_filesand the--ignoreprefixes frompytest.iniand selects:foo_test.pyof a changedfoo.py.It drops tests that no longer exist (fix(hooks): pre-push Phase 0c pytest selection includes files deleted by the push, breaking collection #15751) or that pytest.ini ignores (Per-node service status: templated units are dropped and 'active (exited)' maps to unknown #16019), and it prints
test<TAB>reasonfor each selection.tools/git-hooks/pre-push:The bash
existing_py_filesandpytest_ignoredfunctions are gone, since the helper does both.Acceptance criteria
python_files(read frompytest.ini) is selected, subject to the ignore filterselect()andpytest_settings();test_the_naming_conventions_come_from_pytest_inireads the realpytest.ini.test_*.pyis no longer treated as a production file needing a siblingtest_a_changed_test_module_is_not_given_a_siblingtests/test_x.pyselectedtest_the_16700_changeset_runs_every_test_it_changedreplays #16700's changeset: threetests/test_*.pyfiles and a*_test.py.Refs.Verification
9251f0d91,52a3638cb,a5ed57ba9andb3310df4f:all relevant tests passontools/lint/prepush_selection_test.pyeach time. The installed hook is the pre-fix one, so it didn't selecttests/test_services_stub_is_a_package_16722.py; that's ci(pre-push): a changed test file named test_*.py never runs in the pre-push hook #16711's own bug. CI runs it.bash -non the hook passed.Model Used
Claude Opus 5 (
claude-opus-5).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Developer Experience
Tests