Skip to content

Assert no unexpected interim responses were received#174

Merged
mnot merged 1 commit into
mainfrom
fix/interim-not-cached-assertion
Jun 9, 2026
Merged

Assert no unexpected interim responses were received#174
mnot merged 1 commit into
mainfrom
fix/interim-not-cached-assertion

Conversation

@mnot

@mnot mnot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Fixes #168: the interim-response check only verified that expected interim responses were present. An empty expected_interim_responses: [] therefore asserted nothing, so the kind: required test interim-not-cached could not catch a cache that stored and replayed a 103 interim on a cache hit.

This adds a count assertion that the number of interim responses received equals the number expected. It only fires when expected_interim_responses is present, so interim-102/interim-103 (exactly one interim) and interim-no-header-reuse (no expected_interim_responses on the hit) are unaffected.

Closes #168


🤖 This PR was generated by an AI agent (Claude Code) under human supervision, as part of a maintainer-directed review of the test suite.

The interim-response check only verified that expected interims were
present; an empty expected_interim_responses array asserted nothing,
so interim-not-cached (kind: required) could not catch a cache that
replayed a stored interim. Add a count check that the number of
received interims matches the number expected.

Closes #168

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mnot

mnot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Review: count assertion for interim responses

I traced the change against checkResponse (test-engine/client/test.mjs), the dispatcher setup in makeTest, setupCheck/assert (test-engine/lib/utils.mjs, test-engine/client/utils.mjs), and all four tests in tests/interim.mjs. The fix is correct and I found no regression.

The bug it fixes

interim-not-cached (kind: required) sets expected_interim_responses: [] on the cache-hit request. The old code only iterated that array with forEach, so an empty array asserted nothing — a cache that stored and replayed the 103 would still pass. The added interimResponses.length === expected.length assertion now enforces the requirement: 0 === 0 for a compliant cache, 1 === 0 (real AssertionError) for a non-compliant one. Good.

Placement / semantics

  • The assert is inside the if ('expected_interim_responses' in reqConfig) block and after the forEach. Correct — it never runs when the key is absent.
  • isSetup comes from setupCheck, which is true only for setup: true or setup_tests membership. None of the relevant requests set either, so isSetup is false and a count mismatch throws a hard AssertionError, not a soft SetupError. Correct for these required/optimal hit requests.

No regression on the other tests

  • interim-102 / interim-103: the dispatcher is installed whenever expected_interim_responses is present (makeTest lines 37-42), including the setup request — so the single origin-generated interim is collected and 1 === 1 passes.
  • interim-no-header-reuse: the cache-hit request has no expected_interim_responses, so the whole block (and the new assert) is skipped. Unaffected.

"Could a compliant cache get more interims and be wrongly failed?"

The server emits exactly reqConfig.interim_responses (handle-test.mjs:46) and these are plain GETs without Expect: 100-continue, so no spurious 100 Continue arises. There's no scenario here where a compliant cache legitimately forwards more interims than configured, so exact equality is safe. (If a future test ever needed "at least N" semantics it would need a different check, but that's out of scope.)

Verdict

Approve. Minimal, correctly scoped, closes the gap in #168 without affecting the other three tests.


AI-generated review (Claude Code), produced as part of a maintainer-directed review of the test suite.

@mnot mnot merged commit e350c1d into main Jun 9, 2026
2 checks passed
@mnot mnot deleted the fix/interim-not-cached-assertion branch June 9, 2026 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

interim-not-cached (kind: required) asserts nothing

1 participant