kokhlo commented on issue #44436: URL: https://github.com/apache/superset/issues/44436#issuecomment-5739134602
Taking this one — it's the two test drivers tripping over the locking re-read #44015 added upstream of every patched seam, not a regression in `_do_restore()`. Confirmed shape: both drivers feed `run()` a bare `MagicMock()` entity, so `entity.id` binds as a mock into the `populate_existing().with_for_update()` re-read, producing `sqlite3.ProgrammingError`, which is caught by `@transaction(catches=(SQLAlchemyError, LookupError))` and re-raised as `failed_exc` — before the patched `resolve_version` / `restore_version` side effects are ever reached. That yields exactly the six failures: `__cause__` is the `ProgrammingError` instead of `lookup`, and the other driver gets a translated `*UpdateFailedError` instead of its RuntimeError. Plan (test-side, as suggested): give the mock entity a real integer `id` and stub the locking re-read inside the shared `_validate_context` helper (or a sibling context manager the run() drivers opt into), so `run()` reaches `resolve_version` where the tests' side effects live. The row lock itself stays untouched — it closes four concurrency races by design. Will verify against the two passing `app_context` tests in the same file so the fix stays minimal, and run the full `test_base_restore_version_command.py` suite plus the versioning suites that touch these commands. PR within the hour. -- 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]
