You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "kokhlo (via GitHub)" <gi...@apache.org> on 2026/09/19 03:52:46 UTC
[PR] test(versioning): stub the locking re-read so run() reaches the patched restore seams [superset]
kokhlo opened a new pull request, #44437:
URL: https://github.com/apache/superset/pull/44437
## Summary
Fixes the six master-red tests in `tests/unit_tests/commands/test_base_restore_version_command.py` (#44436) so `unit-tests-required` stops reddening every PR built against master's tip.
The two failing drivers feed `run()` a bare `MagicMock()` entity. Since #44015, `_do_restore()` issues a real locking re-read (`populate_existing().with_for_update()`) *before* `resolve_version`, and the mock's auto-created `id` cannot bind into that query (`sqlite3.ProgrammingError`). The `@transaction(catches=(SQLAlchemyError, LookupError))` wrapper translates the binding error into `failed_exc`, so the patched `resolve_version` / `restore_version` seams 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_context` now gives the mock entity a real integer `id` and stubs `superset.commands.version_restore.db` with a self-returning query chain whose `one_or_none()` hands the entity back — the same wiring `test_restore_version_concurrency.py` already uses for the lock itself. `validate()`-only tests are unaffected; the row lock in `_do_restore()` is untouched.
## Test plan
- [x] RED on pristine master: exactly the six tests from the issue fail (`6 failed, 6 passed`)
- [x] GREEN with the patch: `12 passed` for the full file
- [x] Neighbours unaffected: `test_restore_version_concurrency.py` + `tests/unit_tests/versioning` = `291 passed`
- [x] `ruff check` + `ruff format --check` clean
Closes #44436
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(versioning): stub the locking re-read so run() reaches the patched restore seams [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #44437:
URL: https://github.com/apache/superset/pull/44437#discussion_r4064679619
##########
tests/unit_tests/commands/test_base_restore_version_command.py:
##########
@@ -53,16 +53,31 @@ def _validate_context(entity: MagicMock) -> Iterator[None]:
is_managed_externally guard: capture is on, the entity is found, and the
editorship check passes. What varies between tests is only the entity's
``is_managed_externally`` value.
+
+ The mock entity also carries a real integer ``id``, and the locking
+ re-read ``run()`` performs after ``validate()`` is stubbed to hand the
+ entity back: a bare MagicMock id cannot bind into the FOR UPDATE query
+ (``sqlite3.ProgrammingError``), which the transaction wrapper would
+ translate into ``failed_exc`` before the patched ``resolve_version`` /
+ ``restore_version`` seams are ever reached.
"""
+ entity.id = 1
with (
patch("superset.commands.version_restore.capture_enabled", return_value=True),
patch(
"superset.commands.version_restore.find_active_by_uuid",
return_value=entity,
),
patch("superset.commands.version_restore.security_manager") as mock_sec,
+ 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
Review Comment:
The `db` patch is a broad, un-spec’d `MagicMock`, which can silently accept typos or drift from the real query chain. Consider using `autospec=True` (or a spec/spec_set on `mock_db.session` / `query`) so failures surface when the production code changes (e.g., a different query method is called), rather than silently passing.
##########
tests/unit_tests/commands/test_base_restore_version_command.py:
##########
@@ -53,16 +53,31 @@ def _validate_context(entity: MagicMock) -> Iterator[None]:
is_managed_externally guard: capture is on, the entity is found, and the
editorship check passes. What varies between tests is only the entity's
``is_managed_externally`` value.
+
+ The mock entity also carries a real integer ``id``, and the locking
+ re-read ``run()`` performs after ``validate()`` is stubbed to hand the
+ entity back: a bare MagicMock id cannot bind into the FOR UPDATE query
+ (``sqlite3.ProgrammingError``), which the transaction wrapper would
+ translate into ``failed_exc`` before the patched ``resolve_version`` /
+ ``restore_version`` seams are ever reached.
"""
+ entity.id = 1
Review Comment:
`_validate_context` now overwrites `entity.id` unconditionally. This can make future tests brittle if a test intentionally sets a specific integer `id` (e.g., to assert query arguments). Consider only assigning when the current value is not already an `int` (e.g., when it’s the default `MagicMock` attribute), so preconfigured IDs are preserved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(versioning): stub the locking re-read so run() reaches the patched restore seams [superset]
Posted by "kokhlo (via GitHub)" <gi...@apache.org>.
kokhlo commented on PR #44437:
URL: https://github.com/apache/superset/pull/44437#issuecomment-5739326391
The only red check is `test-postgres-hive`, which failed in its `Start hadoop and hive` setup step before any test ran:
```
Error response from daemon: failed to set up container networking: driver failed
programming external connectivity on endpoint namenode (...):
failed to bind host port 0.0.0.0:50070/tcp: address already in use
```
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(versioning): stub the locking re-read so run() reaches the patched restore seams [superset]
Posted by "kokhlo (via GitHub)" <gi...@apache.org>.
kokhlo commented on PR #44437:
URL: https://github.com/apache/superset/pull/44437#issuecomment-5767387513
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(versioning): stub the locking re-read so run() reaches the patched restore seams [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44437:
URL: https://github.com/apache/superset/pull/44437#issuecomment-5739157856
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(versioning): stub the locking re-read so run() reaches the patched restore seams [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44437:
URL: https://github.com/apache/superset/pull/44437#issuecomment-5739186198
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44437?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 57.31%. Comparing base ([`128c0c1`](https://app.codecov.io/gh/apache/superset/commit/128c0c159ab8a1157e1f0688b43ba16b0c034889?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9bc5999`](https://app.codecov.io/gh/apache/superset/commit/9bc5999370aa38e88fb088810c9d60340fec2ee2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (128c0c1) and HEAD (9bc5999). Click for more details.
>
> <details><summary>HEAD has 38 uploads less than BASE</summary>
>
>| Flag | BASE (128c0c1) | HEAD (9bc5999) |
>|------|------|------|
>|python|33|1|
>|presto|7|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44437 +/- ##
==========================================
- Coverage 66.29% 57.31% -8.98%
==========================================
Files 2942 2942
Lines 175692 175692
Branches 40788 40788
==========================================
- Hits 116467 100705 -15762
- Misses 56770 74027 +17257
+ Partials 2455 960 -1495
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44437/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.04% <ø> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/44437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.04% <ø> (-17.68%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44437?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(versioning): stub the locking re-read so run() reaches the patched restore seams [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on PR #44437:
URL: https://github.com/apache/superset/pull/44437#issuecomment-5767141281
Thanks for the quick turnaround on #44436, @kokhlo, and your diagnosis is exactly right.
The catch is that master already carries an equivalent fix: `43fee87672` (#44445, merged 2026-09-19) added a sibling `_restore_context` helper to the same file. Your branch was opened about four hours before that one landed, so this is a scoop rather than a mistake, but the remediation has shipped. I verified it on a clean checkout: at this PR's parent `128c0c1` the six tests fail with `sqlite3.ProgrammingError: Error binding parameter 1: type 'MagicMock' is not supported`, exactly as you described, and at master `322eaeaf99` `test_base_restore_version_command.py` plus `test_restore_version_concurrency.py` give `17 passed`. So I agree with @rebenitez1802 that the right move here is to close this one as superseded.
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 `unit-tests-required` will read the issue and conclude master is still broken.
For the record, since stubbing out a lock read always invites the question: no coverage was lost by either fix. `test_restore_version_concurrency.py` pins the lock directly, and I confirmed by mutation on master that dropping `with_for_update()`, `populate_existing()`, the `uuid` pin or the `deleted_at=None` predicate from `_do_restore` each turns that file red (`3 failed, 2 passed`) while `test_base_restore_version_command.py` stays green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] test(versioning): stub the locking re-read so run() reaches the patched restore seams [superset]
Posted by "kokhlo (via GitHub)" <gi...@apache.org>.
kokhlo closed pull request #44437: test(versioning): stub the locking re-read so run() reaches the patched restore seams
URL: https://github.com/apache/superset/pull/44437
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org