SameerMesiah97 commented on code in PR #67626:
URL: https://github.com/apache/airflow/pull/67626#discussion_r3314710674
##########
providers/dbt/cloud/tests/unit/dbt/cloud/triggers/test_dbt.py:
##########
@@ -87,6 +89,37 @@ async def test_dbt_run_job_trigger(self,
mock_get_job_status, end_time):
assert task.done() is False
asyncio.get_event_loop().stop()
+ @pytest.mark.asyncio
+
@mock.patch("airflow.providers.dbt.cloud.hooks.dbt.DbtCloudHook.get_job_status")
+ async def test_dbt_run_job_trigger_uses_wall_clock_end_time(self,
mock_get_job_status, monkeypatch):
+ """Assert serialized end_time is compared to wall-clock time, not
monotonic time."""
+
+ clock = MagicMock()
+ clock.time.return_value = 1000.0
+ clock.monotonic.return_value = 10000.0
+ monkeypatch.setattr(dbt_trigger_module, "time", clock)
+
+ mock_get_job_status.return_value = DbtCloudJobRunStatus.RUNNING.value
+ trigger = DbtCloudRunJobTrigger(
+ conn_id=self.CONN_ID,
+ poll_interval=self.POLL_INTERVAL,
+ end_time=1060.0,
+ run_id=self.RUN_ID,
+ account_id=self.ACCOUNT_ID,
+ )
+
+ task = asyncio.create_task(trigger.run().__anext__())
+ await asyncio.sleep(0)
+
+ assert task.done() is False
+ clock.time.assert_called()
+ clock.monotonic.assert_not_called()
+ mock_get_job_status.assert_called_once_with(self.RUN_ID,
self.ACCOUNT_ID)
+
+ task.cancel()
+ with suppress(asyncio.CancelledError):
+ await task
+
Review Comment:
I think this would be better:
```
@pytest.mark.asyncio
@mock.patch("airflow.providers.dbt.cloud.triggers.dbt.time.monotonic")
@mock.patch("airflow.providers.dbt.cloud.triggers.dbt.time.time")
@mock.patch("airflow.providers.dbt.cloud.hooks.dbt.DbtCloudHook.get_job_status")
async def test_dbt_run_job_trigger_uses_wall_clock_end_time(
self,
mock_get_job_status,
mock_time,
mock_monotonic,
end_time,
):
"""Assert serialized end_time is compared to wall-clock time, not
monotonic time."""
mock_time.return_value = end_time - 60
mock_get_job_status.return_value = DbtCloudJobRunStatus.RUNNING.value
trigger = DbtCloudRunJobTrigger(
conn_id=self.CONN_ID,
poll_interval=self.POLL_INTERVAL,
end_time=end_time,
run_id=self.RUN_ID,
account_id=self.ACCOUNT_ID,
)
task = asyncio.create_task(trigger.run().__anext__())
await asyncio.sleep(0)
# TriggerEvent was not returned.
assert task.done() is False
mock_time.assert_called()
mock_monotonic.assert_not_called()
mock_get_job_status.assert_called_once_with(
self.RUN_ID,
self.ACCOUNT_ID,
)
asyncio.get_event_loop().stop()
```
##########
providers/dbt/cloud/tests/unit/dbt/cloud/triggers/test_dbt.py:
##########
@@ -18,12 +18,14 @@
import asyncio
import time
+from contextlib import suppress
from unittest import mock
-from unittest.mock import AsyncMock
+from unittest.mock import AsyncMock, MagicMock
import pytest
from airflow.providers.dbt.cloud.hooks.dbt import DbtCloudHook,
DbtCloudJobRunStatus
+from airflow.providers.dbt.cloud.triggers import dbt as dbt_trigger_module
Review Comment:
This import won't be needed once you adjust the test.
##########
providers/dbt/cloud/tests/unit/dbt/cloud/operators/test_dbt.py:
##########
@@ -210,7 +210,8 @@ def
test_execute_deferrable_does_not_pass_execution_timeout_to_defer(
execution_timeout=timedelta(seconds=3),
)
- dbt_op.execute(MagicMock())
+ with patch("airflow.providers.dbt.cloud.operators.dbt.time.time",
return_value=1000.0):
+ dbt_op.execute(MagicMock())
Review Comment:
This change is not strictly needed as the purpose of this test was to ensure
that a non-None value is not passed as a parameter to the `execution_timeout`
argument (it was causing a premature timeout preventing the DBT job from being
cancelled on reaching the `execution_deadline`). So the actual value is not
relevant (that's why `ANY` was used before). Not a blocker.
--
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]
