feat(testing): make SellerA2AClient event drain configurable#829
Merged
Conversation
There was a problem hiding this comment.
LGTM. Additive kwarg with a default that preserves the prior range(32) — non-breaking on the public testing surface, conventional-commit prefix matches.
Things I checked
src/adcp/testing/harness.py:296— defaultmax_events=32matches the prior hardcoded bound. Loop at L356 and error message at L370-L374 are consistent.SellerA2AClientis re-exported fromadcp.testing(src/adcp/testing/__init__.py:26,48) — public surface. Change is additive, no!/BREAKING CHANGE:needed.- Regression coverage covers both arms: default-cap exhaustion at
tests/test_seller_a2a_client.py:170-177, larger cap success at L181-L191. - Incidental
supported_billing=("operator",)→supported_billing=["operator"]in the shared fixture aligns with the declaredlist[str]type atsrc/adcp/decisioning/platform.py:198. Drift cleanup, not a hidden break.
Follow-ups (non-blocking — file as issues)
- The new error message format
"within {timeout_seconds}s x {max_events} events"(src/adcp/testing/harness.py:372) uses a barexas a separator. Reads fine to humans; a log scanner could mis-tokenize it next to a float. Something like"within {timeout_seconds}s per event, max {max_events} events"would be unambiguous. Purely cosmetic.
Minor nits (non-blocking)
- Substring assertion is implicitly format-coupled.
tests/test_seller_a2a_client.py:177asserts"within 0.01s x 32 events". Stable today because CPython's shortest-round-trip repr of0.01is"0.01", but if anyone ever adds a format spec to the f-string the test silently passes for0.01and breaks for other values. Splitting into separate"x 32 events"and"0.01"substring checks would be more robust.
Mirroring #763 onto the base branch for the CodeQL gate is the right move — happy path is unchanged, regression coverage is in place. Safe to merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Mirrors the contribution from #763 onto a branch in the base repository so the repository's required CodeQL check can run.
Closes #698.
Supersedes #763.
What changed
max_events: int = 32toSellerA2AClient.invoke().timeout_secondsandmax_events.Original patch authored by @sangilish in #763.
Testing
uv run --extra dev python -m pytest tests/test_seller_a2a_client.py -quv run --extra dev ruff check src/adcp/testing/harness.py tests/test_seller_a2a_client.pyuv run --extra dev mypy src/adcp/testing tests/test_seller_a2a_client.py