nagasrisai commented on code in PR #64614:
URL: https://github.com/apache/airflow/pull/64614#discussion_r3070365332
##########
task-sdk/src/airflow/sdk/execution_time/task_runner.py:
##########
@@ -415,7 +415,7 @@ def xcom_pull(
)
if values is None:
- xcoms.append(None)
+ xcoms.append(default)
Review Comment:
Thanks for the kind words! Glad it was easy to follow.
##########
task-sdk/tests/task_sdk/execution_time/test_task_runner.py:
##########
@@ -2106,6 +2106,30 @@ def mock_send_side_effect(*args, **kwargs):
),
)
+ def test_xcom_pull_default_respected_when_no_map_indexes(
+ self,
+ create_runtime_ti,
+ mock_supervisor_comms,
+ ):
+ """Test that ``default`` is returned when map_indexes is not specified
and no XCom is found.
+
+ Previously, ``xcoms.append(None)`` was used unconditionally on the
no-map-indexes
+ path, so a user-supplied ``default`` was silently ignored.
+ """
+
+ class CustomOperator(BaseOperator):
+ def execute(self, context):
+ pass
+
+ task = CustomOperator(task_id="pull_task")
+ runtime_ti = create_runtime_ti(task=task)
+
+ with patch.object(XCom, "get_all") as mock_get_all:
+ mock_get_all.return_value = None
+ result = runtime_ti.xcom_pull(key="missing_key",
task_ids="task_a", default="fallback_value")
+
+ assert result == "fallback_value"
+
Review Comment:
Done. Added
`test_xcom_pull_default_fills_correct_position_with_multiple_task_ids` — it
pulls from `["task_with_xcom", "task_without_xcom"]` where the first has a
stored value and the second returns nothing, and checks the result comes back
as `["stored_value", "my_default"]`. That makes it explicit that the default
lands in the right slot and doesn't bleed into positions where there was actual
data.
##########
task-sdk/tests/task_sdk/execution_time/test_task_runner.py:
##########
@@ -2106,6 +2106,30 @@ def mock_send_side_effect(*args, **kwargs):
),
)
+ def test_xcom_pull_default_respected_when_no_map_indexes(
Review Comment:
Good point. Added
`test_xcom_pull_default_is_none_when_not_passed_and_no_xcom` which calls
`xcom_pull` without a `default` argument and asserts the result is `None` when
nothing is stored. That pins down that the old behaviour is still intact.
--
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]
