Add deterministic async transport contract tests - #316
Conversation
|
@claude please do a run |
|
Claude finished @Mattsface's task in 1m 33s —— View job Run results for
|
| Job | Result |
|---|---|
| Offline tests — Python 3.10 | ✅ pass |
| Offline tests — Python 3.11 | ✅ pass |
| Offline tests — Python 3.12 | ✅ pass |
| Offline tests — Python 3.13 | ✅ pass |
| Offline tests — Python 3.14 | ✅ pass |
| Sync-only installation | ✅ pass |
| Build and validate package | ✅ pass |
Full suite result (Python 3.12 job, representative of all matrix legs): 712 passed, 0 failed, 29 warnings in 26.06s. The warnings are expected MlbHttpCompatibilityWarning emissions from the existing test_http_contract.py compatibility-mode tests, not from the new async file, and not failures.
On lint/type-check: This repo's build-and-test.yml workflow doesn't currently define a separate lint or type-check job (no ruff/mypy/flake8 config in pyproject.toml either) — "Offline CI" here is tests + sync-only install verification + package build/validation only. So there's nothing additional to run on that front beyond what's already green above.
Summary
All 6 commits in this PR (2510a6f through e7a4bdd) pass the full offline suite across every supported Python version (3.10–3.14), the sync-only install still works without HTTPX, and the package builds/validates cleanly. No production code is touched, consistent with the PR description. Nothing failed — this PR is green and ready from a test-execution standpoint.
If you'd like me to actually execute poetry run pytest from inside this session (rather than reading the already-completed CI run), the --allowedTools for this job would need to permit Bash(poetry:*) / Bash(python3 -m *).
· Branch
Covers the remaining HTTP/result and error/warning contract gaps from #298 for AsyncMlbDataAdapter, without duplicating the suite added in #301/#314: - final non-404 4xx (403) raises MlbHttpError under strict_http=True, with structured status/reason/URL/method context - final non-404 4xx under strict_http=False emits one MlbHttpCompatibilityWarning and returns the historical empty MlbResult - compatibility warnings do not leak response bodies or headers - compatibility warnings are attributed to the awaiting caller's call site - a failure while extracting optional error-response context degrades that field instead of replacing the original MlbHttpError Test-only change. Existing helpers (run_async, _ScriptedHandler, _response, _owned_adapter, SLEEP_TARGET) and httpx.MockTransport are reused; no live MLB API requests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EHYA36k526xUzTUxx4iPSZ
The default retry policy uses total=3, connect=3 and status=3, so the existing exhaustion tests that observe four attempts cannot tell those budgets apart, and the generic timeout/request-error branches had no coverage at all. Narrow one budget per test so the observed attempt count is uniquely attributable to it: - generic failures (pool timeout, read error) spend the total budget and still surface MlbTimeoutError / MlbTransportError with the original cause - a connection failure spends the connect budget, not the total one - a retryable status spends the status budget, not the total one Test-only change; no production behavior was modified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EHYA36k526xUzTUxx4iPSZ
httpx.ReadError currently falls through to the generic RequestError branch and so spends the total budget, but #298 does not define that mapping, and asserting it would freeze an implementation detail as public contract. The read budget already has deterministic coverage through ReadTimeout, and the pool timeout case is enough to prove the generic timeout path spends the total budget. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EHYA36k526xUzTUxx4iPSZ
The suite already proved two concurrent requests can share one adapter and that cancelling one does not cancel another. These lock down the remaining #298 concurrency promises: - concurrent requests keep their own ep_params and their own response, now asserted against the query the transport actually observed - a request that exhausts its retry budget and raises MlbHttpError leaves an unrelated concurrent request untouched, on its single attempt - a second request completes while the first is parked inside its retry backoff, proven with asyncio.Event synchronization rather than wall-clock timing, and bounded so a serializing regression fails fast instead of hanging CI Test-only change; no production behavior was modified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EHYA36k526xUzTUxx4iPSZ
test_library_owned_client_closes only covers an adapter that never issued a request, so nothing asserted that a used adapter is still closable. Cover the three states a request can leave behind: - after a successful request, aclose() closes the library-owned client - after a request that raised MlbHttpError, cleanup succeeds and the error's public fields are unchanged - after an in-flight request is cancelled, CancelledError stays the caller's outcome and cleanup still closes the client The cancellation test waits on an event set inside the transport handler, so the request is genuinely in flight before it is cancelled. Test-only change; no production behavior was modified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EHYA36k526xUzTUxx4iPSZ
The async suite proved the 5xx raise only under the default strict adapter; every strict_http=False test targeted a 4xx. Nothing stopped a regression that widened compatibility-mode suppression from the 4xx branch into the 5xx branch, which would have returned a warned empty MlbResult with the suite still green. A persistent 503 against a strict_http=False adapter still raises MlbHttpError after the full status retry budget, and emits no MlbHttpCompatibilityWarning. Test-only change; no production behavior was modified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EHYA36k526xUzTUxx4iPSZ
e7a4bdd to
28528a1
Compare
|
@claude try again now that you have more permissions |
|
Claude finished @Mattsface's task in 1m 3s —— View job Run results for
|
| Job | Result |
|---|---|
| Sync-only installation | ✅ pass |
| Offline tests — Python 3.10 | ✅ pass |
| Offline tests — Python 3.11 | ✅ pass |
| Offline tests — Python 3.12 | ✅ pass |
| Offline tests — Python 3.13 | ✅ pass |
| Offline tests — Python 3.14 | ✅ pass |
| Build and validate package | ✅ pass |
All jobs completed with no failed steps on the exact commit currently at the tip of this branch (28528a1, "test: prove a final 5xx raises regardless of strict_http").
Diff check: git diff origin/release/1.1.0...HEAD --stat confirms only tests/test_async_mlb_dataadapter.py is touched (+430/-2), matching the PR description — no production code changed.
On lint/type-check: This repo's build-and-test.yml workflow still doesn't define a separate lint/type-check job (no ruff/mypy config in pyproject.toml), so "Offline CI" = tests + sync-only install verification + package build/validation, and that's all green.
Summary
This PR is green on its current HEAD across all supported Python versions (3.10–3.14), the sync-only install still works without HTTPX, and the package builds/validates cleanly. Only the test file changed, consistent with the PR description. Nothing failed.
If you want me to actually execute poetry run pytest from inside this session rather than reading CI results, the --allowedTools for this job would need to permit Bash(poetry:*) and/or Bash(python3 -m *) for non-interactive auto-approval.
· Branch
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
2 participants
Why
The async transport introduced for v1.1 needs deterministic coverage of the behavioral contract defined in #298.
Without dedicated contract tests, future changes to AsyncMlbDataAdapter could drift from the existing synchronous 1.x behavior around HTTP errors, retries, cancellation, client ownership, cleanup, and concurrency.
This PR adds offline contract coverage for AsyncMlbDataAdapter before the higher-level AsyncMlb client is introduced in #303.
Closes #302.
What
Adds deterministic async transport tests covering:
The tests use httpx.MockTransport and remain fully offline.
No production code is changed by this PR.
AsyncMlb context-manager and endpoint-level lifecycle behavior remains intentionally deferred to #303.
Tests
The async transport tests use mocked HTTP responses and exceptions and do not contact the live MLB API.
Validation includes:
tests/test_async_mlb_dataadapter.py
along with the broader repository test suite and existing lint/type-check workflow.
The new coverage was added incrementally and reviewed against the contract in #298 and the existing synchronous HTTP/retry behavior.
Risk and impact
Risk: Minimal
This PR changes tests only. It does not modify runtime behavior, public APIs, transport implementation, or synchronous behavior.
The primary risk is over-constraining implementation details through overly specific tests. The added tests intentionally focus on observable public behavior and avoid depending on HTTPX internals where possible.
If something does go wrong with this PR, the likely impact is a false-positive or overly strict test failure during development or CI. It should not affect users of the library or production behavior.