Re: [PR] Fix ExternalTaskSensor to use timeout parameter in deferrable mode (#… [airflow]

["iveselin-glooko (via GitHub)" <gi...@apache.org>]

iveselin-glooko commented on code in PR #62556:
URL: https://github.com/apache/airflow/pull/62556#discussion_r2870891384


##########
providers/standard/tests/unit/standard/sensors/test_external_task_sensor.py:
##########
@@ -1049,6 +1049,54 @@ def test_external_task_sensor_deferrable(self, dag_maker):
         assert exc.value.trigger.external_task_ids == ["test_task"]
         assert exc.value.trigger.execution_dates == [DEFAULT_DATE]
 
+    @pytest.mark.execution_timeout(10)
+    def test_external_task_sensor_deferrable_uses_timeout(self, dag_maker):
+        """Test that deferrable mode uses timeout parameter instead of execution_timeout."""
+        context = {"execution_date": DEFAULT_DATE}
+        with dag_maker() as dag:
+            op = ExternalTaskSensor(
+                task_id="test_external_task_sensor_check",
+                external_dag_id="test_dag_parent",
+                external_task_id="test_task",
+                deferrable=True,
+                timeout=60,
+                execution_timeout=timedelta(seconds=120),

Review Comment:
   These two specs both set `timeout` and `execution_timeout` and end up testing the same thing - that `timeout` is prioritized over `execution_timeout`. Either remove or change one to test different conditions. 
   Ideally, you would have 3 test cases:
   1. timeout set, execution_timeout not set
   2. timeout not set, execution_timeout set
   3. timeout set, execution_timeout set



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org