shahar1 commented on code in PR #71528:
URL: https://github.com/apache/airflow/pull/71528#discussion_r4090748827
##########
providers/google/tests/unit/google/cloud/operators/test_kubernetes_engine.py:
##########
@@ -1060,12 +1061,145 @@ def test_invoke_defer_method(
logging_interval=None,
last_log_time=mock_last_log_time,
use_dns_endpoint=False,
+ trigger_kwargs={},
)
mock_defer.assert_called_once_with(
trigger=mock_trigger.return_value,
method_name="trigger_reentry",
+ timeout=None,
)
+ @mock.patch(GKE_OPERATORS_PATH.format("GKEStartPodOperator.defer"))
+
@mock.patch(GKE_OPERATORS_PATH.format("GKEClusterAuthDetails.fetch_cluster_info"))
+ @mock.patch(GKE_OPERATORS_PATH.format("GKEHook"))
+ @mock.patch(GKE_OPERATORS_PATH.format("GKEStartPodTrigger"))
+ @mock.patch(GKE_OPERATORS_PATH.format("timezone.utcnow"))
+ def
test_invoke_defer_method_passes_execution_deadline_when_execution_timeout_set(
Review Comment:
These three tests share about 25 lines of operator setup. They differ only
in `execution_timeout`, the pre-existing `trigger_kwargs`, the frozen time, and
the expected values. AGENTS.md says: *"Use `@pytest.mark.parametrize` for
multiple similar inputs — consolidate tests that only differ in input/expected
values into a single parametrized test."*
When you consolidate them, please also add a case for the `max(remaining,
0)` clamp at `kubernetes_engine.py:814`: freeze the time after the deadline and
expect `timeout == datetime.timedelta(seconds=60)`. No test covers that branch
now, and AGENTS.md asks that *"Every changed or added behaviour must have a
test."*
##########
providers/google/tests/unit/google/cloud/operators/test_kubernetes_engine.py:
##########
@@ -1060,12 +1061,145 @@ def test_invoke_defer_method(
logging_interval=None,
last_log_time=mock_last_log_time,
use_dns_endpoint=False,
+ trigger_kwargs={},
)
mock_defer.assert_called_once_with(
trigger=mock_trigger.return_value,
method_name="trigger_reentry",
+ timeout=None,
)
+ @mock.patch(GKE_OPERATORS_PATH.format("GKEStartPodOperator.defer"))
+
@mock.patch(GKE_OPERATORS_PATH.format("GKEClusterAuthDetails.fetch_cluster_info"))
+ @mock.patch(GKE_OPERATORS_PATH.format("GKEHook"))
+ @mock.patch(GKE_OPERATORS_PATH.format("GKEStartPodTrigger"))
+ @mock.patch(GKE_OPERATORS_PATH.format("timezone.utcnow"))
+ def
test_invoke_defer_method_passes_execution_deadline_when_execution_timeout_set(
+ self, mock_utcnow, mock_trigger, mock_cluster_hook,
mock_fetch_cluster_info, mock_defer
+ ):
+ """
+ ``execution_timeout`` is converted into an absolute
``_execution_deadline``
+ anchored on ``ti.start_date`` and propagated to the trigger via
+ ``trigger_kwargs``.
+ """
+ import time_machine
Review Comment:
`import time_machine` is inside the test body here and again at line 1170.
Please move it to the module imports. AGENTS.md says: *"Imports at top of file.
Valid exceptions: circular imports, lazy loading for worker isolation,
`TYPE_CHECKING` blocks."* None of those exceptions applies to a test module.
--
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]
