Skip to content

Commit 331face

Browse files
committed
Remove test-specific error handling from production code
Address reviewer feedback: Remove try-except blocks that were catching ImportError/AttributeError for test environments. Tests should use mocking instead of having special cases in production code. - Remove try-except blocks around XCom.get_value() calls in get_link() - Add proper XCom.get_value() mocking to all tests that call get_link() - Ensure tests properly mock XCom behavior instead of relying on error handling
1 parent 2f8e943 commit 331face

File tree

2 files changed

+25
-16
lines changed

2 files changed

+25
-16
lines changed

providers/standard/src/airflow/providers/standard/operators/trigger_dagrun.py

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -81,12 +81,7 @@ def get_link(self, operator: BaseOperator, *, ti_key: TaskInstanceKey) -> str:
8181
assert isinstance(operator, TriggerDagRunOperator)
8282

8383
# Try to get the resolved dag_id from XCom first (for dynamic dag_ids)
84-
trigger_dag_id = None
85-
try:
86-
trigger_dag_id = XCom.get_value(ti_key=ti_key, key=XCOM_DAG_ID)
87-
except (ImportError, AttributeError):
88-
# Fallback for test environments or when supervisor comms is not available
89-
pass
84+
trigger_dag_id = XCom.get_value(ti_key=ti_key, key=XCOM_DAG_ID)
9085

9186
# Fallback to operator attribute and rendered fields if not in XCom
9287
if not trigger_dag_id:
@@ -99,12 +94,7 @@ def get_link(self, operator: BaseOperator, *, ti_key: TaskInstanceKey) -> str:
9994

10095
# Fetch the correct dag_run_id for the triggerED dag which is
10196
# stored in xcom during execution of the triggerING task.
102-
triggered_dag_run_id = None
103-
try:
104-
triggered_dag_run_id = XCom.get_value(ti_key=ti_key, key=XCOM_RUN_ID)
105-
except (ImportError, AttributeError):
106-
# Fallback for test environments or when supervisor comms is not available
107-
pass
97+
triggered_dag_run_id = XCom.get_value(ti_key=ti_key, key=XCOM_RUN_ID)
10898

10999
if AIRFLOW_V_3_0_PLUS:
110100
from airflow.utils.helpers import build_airflow_dagrun_url

providers/standard/tests/unit/standard/operators/test_trigger_dagrun.py

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -441,8 +441,11 @@ def test_trigger_dagrun_pushes_dag_id_to_xcom(self, dag_maker, session):
441441
run_id_xcom = triggering_ti.xcom_pull(key=XCOM_RUN_ID)
442442
assert run_id_xcom == "test_run_id"
443443

444-
def test_extra_operator_link(self, dag_maker, session):
444+
@mock.patch("airflow.providers.standard.operators.trigger_dagrun.XCom.get_value")
445+
def test_extra_operator_link(self, mock_xcom_get_value, dag_maker, session):
445446
"""Asserts whether the correct extra links url will be created."""
447+
from airflow.providers.standard.operators.trigger_dagrun import XCOM_RUN_ID
448+
446449
with dag_maker(TEST_DAG_ID, default_args={"start_date": DEFAULT_DATE}, serialized=True):
447450
task = TriggerDagRunOperator(
448451
task_id="test_task", trigger_dag_id=TRIGGERED_DAG_ID, trigger_run_id="test_run_id"
@@ -452,6 +455,14 @@ def test_extra_operator_link(self, dag_maker, session):
452455

453456
triggering_ti = session.query(TaskInstance).filter_by(task_id=task.task_id, dag_id=task.dag_id).one()
454457

458+
# Mock XCom.get_value to return None for dag_id but return run_id for XCOM_RUN_ID
459+
def mock_get_value(ti_key, key):
460+
if key == XCOM_RUN_ID:
461+
return "test_run_id"
462+
return None
463+
464+
mock_xcom_get_value.side_effect = mock_get_value
465+
455466
with mock.patch("airflow.utils.helpers.build_airflow_url_with_query") as mock_build_url:
456467
# This is equivalent of a task run calling this and pushing to xcom
457468
task.operator_extra_links[0].get_link(operator=task, ti_key=triggering_ti.key)
@@ -463,7 +474,8 @@ def test_extra_operator_link(self, dag_maker, session):
463474
}
464475
assert expected_args in args
465476

466-
def test_extra_operator_link_with_dynamic_dag_id(self, dag_maker, session):
477+
@mock.patch("airflow.providers.standard.operators.trigger_dagrun.XCom.get_value")
478+
def test_extra_operator_link_with_dynamic_dag_id(self, mock_xcom_get_value, dag_maker, session):
467479
"""Test that operator link works correctly when dag_id is dynamically resolved from XCom."""
468480
from airflow.providers.standard.operators.trigger_dagrun import XCOM_DAG_ID, XCOM_RUN_ID
469481

@@ -479,8 +491,15 @@ def test_extra_operator_link_with_dynamic_dag_id(self, dag_maker, session):
479491

480492
triggering_ti = session.query(TaskInstance).filter_by(task_id=task.task_id, dag_id=task.dag_id).one()
481493

482-
# Simulate that the operator has pushed a dynamic dag_id to XCom
483-
triggering_ti.xcom_push(key=XCOM_DAG_ID, value="dynamic_dag_id")
494+
# Mock XCom.get_value to return our test values
495+
def mock_get_value(ti_key, key):
496+
if key == XCOM_DAG_ID:
497+
return "dynamic_dag_id"
498+
elif key == XCOM_RUN_ID:
499+
return "test_run_id"
500+
return None
501+
502+
mock_xcom_get_value.side_effect = mock_get_value
484503

485504
with mock.patch("airflow.utils.helpers.build_airflow_url_with_query") as mock_build_url:
486505
task.operator_extra_links[0].get_link(operator=task, ti_key=triggering_ti.key)

0 commit comments

Comments
 (0)