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: [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]
