potiuk commented on code in PR #68614: URL: https://github.com/apache/airflow/pull/68614#discussion_r4064564011
########## airflow-e2e-tests/tests/airflow_e2e_tests/basic_tests/test_dag_test_executor.py: ########## @@ -0,0 +1,53 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import pytest + + +class TestDagTestExecutor: + """Verifies that `dag.test()` executes cleanly via CLI and Python SDK across matrix configurations.""" + + @pytest.mark.parametrize("execution_type", ["cli", "python_sdk"]) + @pytest.mark.parametrize("use_executor", ["true", "false"]) + def test_dag_test_matrix_execution(self, compose_instance, execution_type, use_executor): + """ + Executes dag.test via both CLI and Python SDK modes with use_executor=True and False. + This provides a true 4-case end-to-end integration verification. + """ + if execution_type == "cli": + cmd_str = ( + f"USE_EXECUTOR={use_executor} " Review Comment: `airflow dags test` doesn't read a `USE_EXECUTOR` environment variable. It has a real CLI flag — `--use-executor`, defined at `airflow-core/src/airflow/cli/cli_config.py:468-473`. The env var here is only ever consumed by the `__main__` block in `example_dag_test_executor.py:39`, and the CLI path never runs that block. To be precise about why: the `airflow` CLI is argparse, not click. `cli_parser.py` builds an `argparse.ArgumentParser`, and the `Arg` wrapper at `cli_config.py:76-107` forwards only `flags, help, action, default, nargs, type, choices, required, metavar, dest` to `add_argument()` — there is no `envvar` support, and argparse has no environment fallback. (Breeze *is* click-based and does auto-read env vars, which makes this an easy assumption to carry across.) The consequence is that `use_executor="true"` and `use_executor="false"` build the *same string*, so the CLI half of the matrix is one case executed twice and the flag is never exercised. Something like: ```python executor_flag = " --use-executor" if use_executor == "true" else "" cmd_str = f"airflow dags test example_dag_test_executor{executor_flag}" ``` Worth re-running locally after the change — I'd expect the two cases to behave visibly differently, and if they don't, that's worth knowing before this lands. ########## airflow-e2e-tests/tests/airflow_e2e_tests/basic_tests/test_dag_test_executor.py: ########## @@ -0,0 +1,53 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import pytest + + +class TestDagTestExecutor: + """Verifies that `dag.test()` executes cleanly via CLI and Python SDK across matrix configurations.""" + + @pytest.mark.parametrize("execution_type", ["cli", "python_sdk"]) + @pytest.mark.parametrize("use_executor", ["true", "false"]) + def test_dag_test_matrix_execution(self, compose_instance, execution_type, use_executor): + """ + Executes dag.test via both CLI and Python SDK modes with use_executor=True and False. + This provides a true 4-case end-to-end integration verification. + """ + if execution_type == "cli": + cmd_str = ( + f"USE_EXECUTOR={use_executor} " + "EXECUTE_DAG_TEST=true " + "airflow dags test example_dag_test_executor" + ) + else: + cmd_str = ( + f"USE_EXECUTOR={use_executor} " + "EXECUTE_DAG_TEST=true " + "python /opt/airflow/dags/example_dag_test_executor.py" + ) + + stdout, stderr, exit_code = compose_instance.exec_in_container( + service_name="airflow-scheduler", + command=["sh", "-c", cmd_str], + ) + + assert exit_code == 0, ( Review Comment: This is the only assertion, and it can't tell the four cases apart. Remove `use_executor` from the Dag altogether and every case still exits 0 — so the test cannot fail for the reason it was written. The parametrize promises a matrix; the assertion needs to observe something that actually varies with it. Two options that would work: grep `stdout` for the log line the executor path emits and assert it's present for `true` and absent for `false`; or query task instance state through the API client, the way `test_basic_dag_operations.py:31-38` already does in this suite. [AGENTS.md § Testing Standards](https://github.com/apache/airflow/blob/main/AGENTS.md#testing-standards) frames this as *"every test must fail without the PR's change"* — for a test-only PR, the equivalent is that the test has to be capable of failing when the behaviour it names regresses. ########## airflow-e2e-tests/tests/airflow_e2e_tests/dags/example_dag_test_executor.py: ########## @@ -0,0 +1,43 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import os +from datetime import datetime + +from airflow import DAG Review Comment: `from airflow.sdk import DAG` is the supported import — [AGENTS.md § Architecture Boundaries](https://github.com/apache/airflow/blob/main/AGENTS.md#architecture-boundaries) #1, and both neighbours in this folder already use it (`example_xcom_test.py:23`, `example_event_driven.py:28`). ########## airflow-e2e-tests/tests/airflow_e2e_tests/dags/example_dag_test_executor.py: ########## @@ -0,0 +1,43 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import os +from datetime import datetime + +from airflow import DAG +from airflow.providers.standard.operators.empty import EmptyOperator + +with DAG( + dag_id="example_dag_test_executor", + start_date=datetime(2026, 1, 1), + schedule=None, + catchup=False, + tags=["e2e", "dag_test"], +) as dag: + task_start = EmptyOperator(task_id="task_start") + task_end = EmptyOperator(task_id="task_end") + + task_start >> task_end + +if __name__ == "__main__": + execute_dag_test = os.getenv("EXECUTE_DAG_TEST", "true").lower() == "true" Review Comment: `EXECUTE_DAG_TEST` never varies: both branches of the test set it to `true`, and it defaults to `true` when unset. The `if execute_dag_test:` guard can go along with it, which also removes the silent no-op path where the script exits 0 having run nothing — worth avoiding in a file whose test asserts on exit code. -- 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]
