Repository navigation
Conversation
Code Review Agent Run #ecf374Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44437 +/- ##
===========================================
+ Coverage 66.29% 80.65% +14.36%
===========================================
Files 2942 2942
Lines 175692 175692
Branches 40788 40788
===========================================
+ Hits 116468 141703 +25235
+ Misses 56769 31327 -25442
- Partials 2455 2662 +207
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The only red check is Host-port collision on the shared runner, unrelated to this diff (test-only change; the same job is green on other PRs running today). Would a maintainer mind re-running that job, or I can push an empty commit if that's the preferred trigger — happy either way. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR fixes unit tests around BaseRestoreVersionCommand.run() by stubbing the locking “re-read” query and ensuring the mocked entity has a bindable integer id, so patched restore seams (resolve_version / restore_version) are reached.
Changes:
- Assign a real integer
idto the mocked entity in the shared validation context. - Stub
superset.commands.version_restore.dbwith a self-returning query chain that returns the same entity. - Document why the stub is needed (avoids sqlite bind error translated by the transaction wrapper).
| File | Description |
|---|---|
| tests/unit_tests/commands/test_base_restore_version_command.py | Updates shared test context to provide an integer entity id and stub the lock re-read query used by run() |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| translate into ``failed_exc`` before the patched ``resolve_version`` / | ||
| ``restore_version`` seams are ever reached. | ||
| """ | ||
| entity.id = 1 |
| patch("superset.commands.version_restore.db") as mock_db, | ||
| ): | ||
| mock_sec.raise_for_editorship = MagicMock(return_value=None) | ||
| query = mock_db.session.query.return_value | ||
| query.populate_existing.return_value = query | ||
| query.enable_eagerloads.return_value = query | ||
| query.filter_by.return_value = query | ||
| query.with_for_update.return_value = query | ||
| query.one_or_none.return_value = entity |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Verdict: request changes — recommend closing as superseded. The fix is correct in direction and green in isolation, but the six red tests it targets (#44436) were already fixed on master by #44445 (43fee87672, merged 2026-09-19) with a cleaner, stronger implementation. Merging this now adds redundant, conflicting mocking and drops coverage.
🔴 Blocker — Already fixed on master by #44445
git log origin/master -- tests/unit_tests/commands/test_base_restore_version_command.py shows 43fee87672 … (#44445) at HEAD, fixing the same #44436 red tests. This PR's head does not contain it. The remediation has already shipped.
🔴 Blocker — Silent "Frankenstein" auto-merge (no conflict to flag it)
git merge-tree merges the two versions cleanly (exit 0). The result stacks both fixes: _validate_context patches the whole db module and #44445's _restore_context patches db.session.query again on top — double-mocking. Worse, this PR's new entity.id = 1 clobbers #44445's explicit MagicMock(id=123). The merged file passes 12/12 only by luck (no surviving assertion reads entity.id).
🔴 Blocker — Regresses assertions that master already has
Master's _restore_context-based tests assert restore.assert_called_once_with(cmd.model_cls, cmd._uuid, 123, entity=entity) and assert excinfo.value is error. This PR's head has neither, so adopting its file drops existing coverage.
🟠 Major — Over-broad patching in a shared helper
This PR puts the db patch + query chain into _validate_context, which is also used by the two pure-validate() tests. validate() never calls db.session.query (version_restore.py:209-235), so those tests now stub infrastructure they don't exercise. #44445 avoids this by isolating the db stub in a separate _restore_context.
🟠 Major — entity.id = 1 mutates the caller's mock as a side-effect
A context-manager helper overwriting a caller-set attribute is a footgun; it survives only because nothing reads entity.id. The docstring also slightly overstates it as load-bearing — with db fully patched the query never binds, so the id value isn't required for these patched tests to pass.
🟡 Minor — Stub can't catch a lock regression
The run() tests never assert filter_by(id=…, uuid=…, deleted_at=None), with_for_update(), or query(model_cls) were called — one_or_none() blindly returns the entity. A future drop of FOR UPDATE/the deleted_at predicate/the uuid pin (version_restore.py:145-157) would still pass. Despite the description claiming to mirror test_restore_version_concurrency.py, it neither touches nor reproduces that file's filter_by.assert_called_once_with(...) / query.assert_called_once_with(cmd.model_cls) guards.
Fair concessions: the stub chain is exactly correct (populate_existing → enable_eagerloads → filter_by → with_for_update → one_or_none matches _do_restore method-for-method); it is not masking a product bug (the locking re-read is intentional and the sqlite3.ProgrammingError was a genuine test-only artifact); and the docstring explains the why well.
Recommendation: close as superseded by #44445. If #44445 had not already landed, this PR would still need to (a) split the db stub out of _validate_context, (b) drop the caller-mutating entity.id = 1 for per-test ids, and (c) add the restore.assert_called_once_with(...) / excinfo.value is error assertions — i.e. converge on what #44445 already does.
(Verified locally: master file + concurrency file = 17 passed; this PR's head alone = 12 passed; auto-merged result = 12 passed but with contradictory double-mocking.)
|
Thanks for the quick turnaround on #44436, @kokhlo, and your diagnosis is exactly right. The catch is that master already carries an equivalent fix: One loose end that outlives both PRs: #44436 is still open, because #44445 never linked it. Worth closing, otherwise the next person who sees a red For the record, since stubbing out a lock read always invites the question: no coverage was lost by either fix. |
|
Closing as superseded by #44445 — thanks @gabotorresruiz for the clean checkout verification and the mutation analysis on the lock coverage. Agreed on the loose end: linking #44436 so the next person hitting a red unit-tests-required doesn't read it as master still broken. |
Summary
Fixes the six master-red tests in
tests/unit_tests/commands/test_base_restore_version_command.py(#44436) sounit-tests-requiredstops reddening every PR built against master's tip.The two failing drivers feed
run()a bareMagicMock()entity. Since #44015,_do_restore()issues a real locking re-read (populate_existing().with_for_update()) beforeresolve_version, and the mock's auto-createdidcannot bind into that query (sqlite3.ProgrammingError). The@transaction(catches=(SQLAlchemyError, LookupError))wrapper translates the binding error intofailed_exc, so the patchedresolve_version/restore_versionseams are never reached — producing exactly the six reported failures (wrong__cause__, and the "passes through untranslated" driver seeing a translated error).Fix is test-side, per the issue:
_validate_contextnow gives the mock entity a real integeridand stubssuperset.commands.version_restore.dbwith a self-returning query chain whoseone_or_none()hands the entity back — the same wiringtest_restore_version_concurrency.pyalready uses for the lock itself.validate()-only tests are unaffected; the row lock in_do_restore()is untouched.Test plan
6 failed, 6 passed)12 passedfor the full filetest_restore_version_concurrency.py+tests/unit_tests/versioning=291 passedruff check+ruff format --checkcleanCloses #44436