henry3260 commented on code in PR #64054:
URL: https://github.com/apache/airflow/pull/64054#discussion_r3130865768
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_xcoms.py:
##########
@@ -323,6 +324,41 @@ def test_xcom_get_slice_accepts_include_prior_dates(
assert set(response.json()) == set(expected_xcoms)
+ @pytest.mark.parametrize(
+ ("start", "stop", "expected_count_calls"),
+ [
+ pytest.param(None, None, 0, id="no_slice"),
+ pytest.param(5, None, 0, id="positive_start_only"),
+ pytest.param(None, -5, 0, id="negative_stop_only"),
+ pytest.param(-5, None, 0, id="negative_start_only"),
+ pytest.param(5, -3, 1, id="positive_start_negative_stop"),
+ pytest.param(-5, 10, 1, id="negative_start_positive_stop"),
+ pytest.param(-5, -10, 0, id="both_negative_same_sign"),
+ ],
+ )
+ def test_xcom_slice_count_query_called_at_most_once(
+ self, client, dag_maker, session, start, stop, expected_count_calls
+ ):
+ """Test that get_query_count is called at most once (and only when
necessary)
+ for slice requests with negative indices.
+
+ This validates the performance optimization in
get_mapped_xcom_by_slice().
+ """
+ with dag_maker(dag_id="dag"):
+ EmptyOperator(task_id="task")
+ dag_maker.create_dagrun(run_id="runid")
+
+ with
patch("airflow.api_fastapi.execution_api.routes.xcoms.get_query_count") as
mock_count:
+ qs = {}
+ if start is not None:
+ qs["start"] = start
+ if stop is not None:
+ qs["stop"] = stop
+
+
client.get(f"/execution/xcoms/dag/runid/task/test_key/slice?{urllib.parse.urlencode(qs)}")
+
+ assert mock_count.call_count == expected_count_calls
Review Comment:
> Two gaps that weaken this test:
>
> 1. `mock_count` has no `return_value` configured, so it returns a
`MagicMock`. For the `positive_start_negative_stop` and
`negative_start_positive_stop` cases, that propagates through `stop =
total_count + stop` (or `stop - total_count`) and into `query.slice(...)`,
where SQLAlchemy may raise depending on version.
> 2. `client.get(...)` is never inspected, so a 500 from the slice path
still passes the assertion -- the test only protects the count-call
optimization, not the code that actually uses `total_count`.
>
> Suggest `mock_count.return_value = 0` to keep the arithmetic coherent,
plus `assert response.status_code == 200` so this test catches regressions in
the slice branch too.
Yes, you are right. I didn't notice that. Applied!
--
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]
