Copilot commented on code in PR #67226:
URL: https://github.com/apache/airflow/pull/67226#discussion_r3277060913
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py:
##########
@@ -3516,6 +3523,54 @@ def
test_async_kpo_wait_termination_before_cleanup_on_failure(
post_complete_action.assert_called_once()
+@patch(KUB_OP_PATH.format("extract_xcom"))
+@patch(KUB_OP_PATH.format("post_complete_action"))
+@patch(HOOK_CLASS)
+def test_async_trigger_reentry_returns_sidecar_output_for_multiple_outputs(
+ mocked_hook, post_complete_action, mock_extract_xcom
+):
+ """``trigger_reentry`` must return the sidecar output so the task runner
+ runs ``_push_xcom_if_needed``, which honors ``multiple_outputs`` by fanning
+ out the returned dict into per-key XComs. Before #67224 the operator pushed
+ ``return_value`` manually inside the ``finally`` block and returned
+ ``None``, which silently bypassed the runner's fan-out — making
+ ``multiple_outputs=True`` a no-op on deferrable KPO. The sync path's
+ ``execute_sync`` already returns ``result`` (line 760 in pod.py); this
+ test pins the deferrable path to the same contract.
+ """
+ metadata = {"metadata.name": TEST_NAME, "metadata.namespace":
TEST_NAMESPACE}
+ succeeded_state = mock.MagicMock(**metadata, **{"status.phase":
"Succeeded"})
+ mocked_hook.return_value.get_pod.return_value = succeeded_state
+ # ``return_value`` (not ``side_effect``) so the pod-await poll loop can
+ # call this any number of times without exhausting a fixed list.
+ mocked_hook.return_value.core_v1_client.read_namespaced_pod.return_value =
succeeded_state
+
+ sidecar_output = {"export_arn": "arn:aws:dynamodb:::export/x", "s3_uri":
"s3://b/p"}
+ mock_extract_xcom.return_value = sidecar_output
+
+ k = KubernetesPodOperator(task_id="task", deferrable=True,
do_xcom_push=True, multiple_outputs=True)
+ context = create_context(k)
+ context["ti"].xcom_push = MagicMock()
+
+ success_event = {
+ "status": "success",
+ "message": TEST_SUCCESS_MESSAGE,
+ "name": TEST_NAME,
+ "namespace": TEST_NAMESPACE,
+ }
+
+ result = k.trigger_reentry(context, success_event)
+
+ # The dict is returned — this is the fix. The task runner's
+ # ``_push_xcom_if_needed`` (in
``task-sdk/.../execution_time/task_runner.py``)
+ # then handles both ``return_value`` push and the ``multiple_outputs``
+ # per-key fan-out, exercised end-to-end in ``test-sdk/`` unit tests.
+ assert result is sidecar_output
Review Comment:
This assertion uses `is` to compare dicts (object identity). Prefer `==` so
the test validates the returned content and won’t break if `trigger_reentry`
later returns an equivalent copy instead of the same dict instance.
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py:
##########
@@ -3516,6 +3523,54 @@ def
test_async_kpo_wait_termination_before_cleanup_on_failure(
post_complete_action.assert_called_once()
+@patch(KUB_OP_PATH.format("extract_xcom"))
+@patch(KUB_OP_PATH.format("post_complete_action"))
+@patch(HOOK_CLASS)
+def test_async_trigger_reentry_returns_sidecar_output_for_multiple_outputs(
+ mocked_hook, post_complete_action, mock_extract_xcom
+):
+ """``trigger_reentry`` must return the sidecar output so the task runner
+ runs ``_push_xcom_if_needed``, which honors ``multiple_outputs`` by fanning
+ out the returned dict into per-key XComs. Before #67224 the operator pushed
+ ``return_value`` manually inside the ``finally`` block and returned
+ ``None``, which silently bypassed the runner's fan-out — making
+ ``multiple_outputs=True`` a no-op on deferrable KPO. The sync path's
+ ``execute_sync`` already returns ``result`` (line 760 in pod.py); this
+ test pins the deferrable path to the same contract.
+ """
Review Comment:
The new test docstring includes an issue reference (#67224) and a hard-coded
source line reference (“line 760 in pod.py”). Airflow’s test docstrings should
describe behavior rather than track tickets, and line numbers tend to drift as
code changes. Consider shortening the docstring to just the behavioral contract
and move historical context/issue refs to a regular comment (or the PR
description), and avoid mentioning specific line numbers.
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py:
##########
@@ -3451,13 +3451,17 @@ def
test_async_kpo_wait_termination_before_cleanup_on_success(
# check if it gets the pod
mocked_hook.return_value.get_pod.assert_called_once_with(TEST_NAME,
TEST_NAMESPACE)
- # assert that the xcom are extracted/not extracted
+ # On the success path, ``trigger_reentry`` returns the sidecar output and
+ # leaves the XCom push to the task runner's ``_push_xcom_if_needed`` —
+ # see #67224. The operator no longer pushes ``return_value`` manually here.
if do_xcom_push:
mock_extract_xcom.assert_called_once()
- context["ti"].xcom_push.assert_called_with(XCOM_RETURN_KEY,
mock_extract_xcom.return_value)
+ assert result is mock_extract_xcom.return_value
Review Comment:
These assertions use `is` to compare dict return values (object identity).
This is more brittle than necessary and can fail if the implementation later
returns an equivalent copy rather than the same object. Prefer asserting
equality (`==`) to validate the returned payload without coupling the test to
object identity.
--
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]
