Copilot commented on code in PR #64990:
URL: https://github.com/apache/airflow/pull/64990#discussion_r3066476060
##########
providers/google/tests/unit/google/cloud/triggers/test_cloud_composer.py:
##########
@@ -59,6 +59,14 @@
"output_end": True,
"exit_info": {"exit_code": 0, "error": ""},
}
+TEST_DAG_RUNS_RESULT = lambda state, date_key, dag_run_date: [
+ {
+ "dag_id": TEST_COMPOSER_DAG_ID,
+ "dag_run_id": TEST_COMPOSER_DAG_RUN_ID,
+ "state": state,
+ date_key: dag_run_date,
+ }
+]
Review Comment:
Avoid assigning `lambda` to a name in tests; this commonly trips linters
(e.g., E731 in flake8/ruff) and reduces debuggability. Use a small `def
test_dag_runs_result(...):` helper function instead.
```suggestion
def test_dag_runs_result(state, date_key, dag_run_date):
return [
{
"dag_id": TEST_COMPOSER_DAG_ID,
"dag_run_id": TEST_COMPOSER_DAG_RUN_ID,
"state": state,
date_key: dag_run_date,
}
]
```
##########
providers/google/tests/unit/google/cloud/sensors/test_cloud_composer.py:
##########
@@ -53,6 +53,20 @@
"dag_runs": TEST_DAG_RUNS_RESULT(state, date_key, "dag_run_id"),
"total_entries": 1,
}
+TEST_DAG_RUN_EXECUTION_RANGE = [datetime(2024, 5, 22, 11, 0, 0),
datetime(2024, 5, 22, 12, 0, 0)]
+TEST_DAG_RUN_RESULT_WITH_DATE = lambda state, date_key, dag_run_date: {
Review Comment:
These tests use naive `datetime(...)` values for `execution_range`, while
mocked DAG run dates include `+00:00` (timezone-aware). Because the production
code compares via `.timestamp()`, naive timestamps are interpreted in the
runner’s local timezone, making the tests potentially flaky across
environments. Use timezone-aware UTC datetimes for
`TEST_DAG_RUN_EXECUTION_RANGE` (and the `logical_date` you pass in `context`)
to make the range checks deterministic.
##########
providers/google/src/airflow/providers/google/cloud/triggers/cloud_composer.py:
##########
@@ -267,16 +267,16 @@ def _check_dag_runs_states(
start_date: datetime,
end_date: datetime,
) -> bool:
+ found_in_range = False
for dag_run in dag_runs:
- if (
- start_date.timestamp()
- < parser.parse(
- dag_run["execution_date" if self.composer_airflow_version
< 3 else "logical_date"]
- ).timestamp()
- < end_date.timestamp()
- ) and dag_run["state"] not in self.allowed_states:
- return False
- return True
+ dag_run_date = parser.parse(
+ dag_run["execution_date" if self.composer_airflow_version < 3
else "logical_date"]
+ ).timestamp()
+ if start_date.timestamp() < dag_run_date < end_date.timestamp():
+ found_in_range = True
+ if dag_run["state"] not in self.allowed_states:
+ return False
+ return found_in_range
Review Comment:
The `_check_dag_runs_states` logic is now duplicated (nearly identically) in
both the sensor and trigger modules. To reduce the risk of future drift,
consider extracting a shared helper (e.g., a small utility function that takes
`dag_runs`, `allowed_states`, date-key selection, and window bounds) and
reusing it in both places.
##########
providers/google/src/airflow/providers/google/cloud/sensors/cloud_composer.py:
##########
@@ -217,16 +217,16 @@ def _check_dag_runs_states(
start_date: datetime,
end_date: datetime,
) -> bool:
+ found_in_range = False
for dag_run in dag_runs:
- if (
- start_date.timestamp()
- < parser.parse(
- dag_run["execution_date" if self._composer_airflow_version
< 3 else "logical_date"]
- ).timestamp()
- < end_date.timestamp()
- ) and dag_run["state"] not in self.allowed_states:
- return False
- return True
+ dag_run_date = parser.parse(
+ dag_run["execution_date" if self._composer_airflow_version < 3
else "logical_date"]
+ ).timestamp()
+ if start_date.timestamp() < dag_run_date < end_date.timestamp():
+ found_in_range = True
+ if dag_run["state"] not in self.allowed_states:
+ return False
+ return found_in_range
Review Comment:
Within the loop, `start_date.timestamp()` and `end_date.timestamp()` are
recomputed for every DAG run. Cache these as local variables before the loop
(e.g., `start_ts`, `end_ts`) to avoid repeated conversions. (This same pattern
exists in the trigger implementation as well.)
--
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]
