gkneighb opened a new issue, #44436:
URL: https://github.com/apache/superset/issues/44436
### Bug description
Six tests in
`tests/unit_tests/commands/test_base_restore_version_command.py` fail on
current master, which turns the `unit-tests-required` check red on every PR
whose merge ref includes master's tip:
```
test_registry_lookup_error_maps_to_failed_exc[RestoreChartVersionCommand]
test_registry_lookup_error_maps_to_failed_exc[RestoreDashboardVersionCommand]
test_registry_lookup_error_maps_to_failed_exc[RestoreDatasetVersionCommand]
test_other_exceptions_still_pass_through_untranslated[RestoreChartVersionCommand]
test_other_exceptions_still_pass_through_untranslated[RestoreDashboardVersionCommand]
test_other_exceptions_still_pass_through_untranslated[RestoreDatasetVersionCommand]
```
It is not confined to one contributor's branch — the same six fail on
unrelated open PRs from different authors (e.g. #44435 and #43567), each with
61 passed / 3 failed and no other failures.
### Root cause
Both tests drive `BaseRestoreVersionCommand.run()` with a bare `MagicMock()`
entity returned from a patched `validate()`. Since #44015, `_do_restore()`
issues a real locking re-read *before* it reaches `resolve_version`:
```python
entity = (
db.session.query(self.model_cls)
.populate_existing()
.enable_eagerloads(False)
.filter_by(id=entity.id, uuid=self._uuid, deleted_at=None)
.with_for_update()
.one_or_none()
)
```
`entity.id` on a bare MagicMock is an auto-created MagicMock attribute, so
the driver cannot bind it:
```
sqlite3.ProgrammingError: Error binding parameter 1: type 'MagicMock' is not
supported
```
`ProgrammingError` is a `SQLAlchemyError`, and `run()` wraps `_do_restore()`
in
```python
@transaction(on_error=partial(on_error, catches=(SQLAlchemyError,
LookupError), reraise=self.failed_exc))
```
so the binding error is caught and re-raised as `failed_exc`. That produces
exactly the two observed assertion failures:
- `test_registry_lookup_error_maps_to_failed_exc` asserts
`excinfo.value.__cause__ is lookup`, but the patched `restore_version` is never
called — the query fails first — so `__cause__` is the `ProgrammingError`.
- `test_other_exceptions_still_pass_through_untranslated` expects its
patched `RuntimeError("boom")` to propagate untranslated; the DB error precedes
it, so a translated `*UpdateFailedError` surfaces instead.
### Why it appeared now
The locking re-read landed in `7f0ca472e4` (#44015, 2026-09-14). These two
tests landed five days later in `2e1cf8a72c` (#44253, 2026-09-19). They patch
`capture_enabled`, `find_active_by_uuid`, `security_manager`, `resolve_version`
and `restore_version` — everything except the session query #44015 had inserted
upstream of all of them, so they appear to have been written against a `run()`
that reached `resolve_version` directly. Each change is fine alone; together
they are not.
A corroborating detail: the two passing tests in the same file request the
`app_context` fixture, while these two request neither an app context nor a
stubbed session.
### Suggested fix
Test-side, in `test_base_restore_version_command.py`: give the mock entity a
real integer `id` and stub the locking re-read so `run()` reaches
`resolve_version`, where the tests' `side_effect`s live. Extending
`_validate_context` (or adding a sibling context manager for the `run()` tests)
keeps it in one place.
Fixing this in the command instead would mean weakening a row lock that the
surrounding comment documents as closing four concurrency races, so the test
seems like the right place.
### How to reproduce
```
pytest tests/unit_tests/commands/test_base_restore_version_command.py -k
"registry_lookup_error or other_exceptions_still_pass_through"
```
on master at or after `2e1cf8a72c`.
### Screenshots/recordings
N/A — unit test failure, output above.
### Superset version
master / latest-dev
### Python version
3.11
### Node version
I don't know
### Browser
Not applicable
### Additional context
Affected files:
`tests/unit_tests/commands/test_base_restore_version_command.py`,
`superset/commands/version_restore.py` (`_do_restore`, the post-`validate()`
locking query).
Impact: `unit-tests-required` is red on every PR built against current
master, so the failure is easy to misread as belonging to the PR under review.
### Checklist
- [x] I have searched Superset docs and Slack and didn't find a solution to
my problem.
- [x] I have searched the GitHub issue tracker and didn't find a similar bug
report.
- [x] I have checked Superset's logs for errors and if I found a relevant
Python stacktrace, I included it here as text in the "additional context"
section.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
