potiuk commented on code in PR #64614:
URL: https://github.com/apache/airflow/pull/64614#discussion_r4175754757


##########
task-sdk/tests/task_sdk/execution_time/test_task_runner.py:
##########
@@ -2772,6 +2772,88 @@ def mock_send_side_effect(*args, **kwargs):
             ),
         )
 
+    def test_xcom_pull_default_respected_when_no_map_indexes(

Review Comment:
   This test and `test_xcom_pull_default_is_none_when_not_passed_and_no_xcom` 
differ only in input and expected value — could you fold them into one 
`@pytest.mark.parametrize` test?
   
   ---
   Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting



##########
task-sdk/tests/task_sdk/execution_time/test_task_runner.py:
##########
@@ -2772,6 +2772,88 @@ 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:

Review Comment:
   Please use `autospec=True` on the `XCom.get_all` patches.
   
   ---
   Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting



##########
task-sdk/src/airflow/sdk/execution_time/task_runner.py:
##########
@@ -611,7 +614,10 @@ async def axcom_pull(
                     dag_id=dag_id,
                     include_prior_dates=include_prior_dates,
                 )
-                xcoms.append(None) if values is None else xcoms.extend(values)
+                if values is None:
+                    xcoms.append(default)

Review Comment:
   This async branch isn't covered: all new tests use the sync `xcom_pull`. 
Could you add an async case (e.g. parametrize over sync/async, patching 
`XCom.aget_all`)?
   
   ---
   Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting



-- 
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]

Reply via email to