diff --git a/SPEC.md b/SPEC.md index 3c7972c1a9..ce5a777348 100644 --- a/SPEC.md +++ b/SPEC.md @@ -1911,12 +1911,43 @@ either **waiver-backed** (a `rubric-audit.json` waiver ID), **architectural** with no rubric record — tracked work, not an accepted divergence). A PR that closes a gap deletes exactly its own lines. +One fixture, "List operation returns first page with Link header" +(`conformance/tests/pagination.json`, tagged `link-header`), is handled by a +tag branch rather than a named-skip entry, because the exclusion is +architectural: every SDK auto-paginates by design, so its first-page-only +`requestCount` assertion is inapplicable. What that branch excludes differs by +runner, and the difference is deliberate: + +- **Go, Python, Ruby, TypeScript** suppress the `requestCount` ASSERTION only. + The case still runs, and its `statusCode: 200` and `noError` assertions still + fire. This is what lets `requestCount` be asserted as an exact count + everywhere else (#573) without shedding the rest of the case. + +- **Kotlin and Swift** skip the whole CASE, as they always have. Both derive a + response's status from the last mock response the SDK consumed, and an + auto-paginating SDK walks past the end of a one-response queue, so `statusCode` + reports "no response" and the case cannot pass on those two runners. Narrowing + them to the assertion was tried and reverted: `make conformance-kotlin` and + `make conformance-swift` each then report + `FAIL: List operation returns first page with Link header` / + `Expected status code 200, but got no response` and exit 2. Widening their + status model is separate work, not a skip to delete here. + +Note the shape this avoids: #573 first narrowed nothing and instead added the +whole-case skip to all four remaining runners, which left the fixture skipped by +all six — present in `pagination.json`, passing `conformance-fixtures-check` and +`check-fixture-coverage`, and executed by nothing. That is #572's defect one +layer down. Nothing in the build detects a fixture no runner runs; that gap is +tracked as #602. + **Go** (`conformance/runner/go/main.go` `goSDKSkips`) — architectural; same-origin logic is covered by `TestIsSameOrigin` unit tests: - "Mixed-case host and explicit default port stay on the mocked origin" — Go runner dials `configOverrides.baseUrl` directly; its `httptest` mock owns its origin, so origin-interception normalization does not apply. - "Bracketed IPv6 loopback origin stays on the mocked origin" — same as above. -**Python** (`conformance/runner/python/runner.py` `SKIPS`) — none. +**Python** (`conformance/runner/python/runner.py` `SKIPS`) — none. The +`link-header` fixture above runs; only its `requestCount` assertion is +suppressed. **Ruby** (`conformance/runner/ruby/runner.rb` `RUBY_SKIPS`): - "PUT operation is naturally idempotent" — GET-only retry (waiver 2B.3). @@ -1934,13 +1965,11 @@ logic is covered by `TestIsSameOrigin` unit tests: **TypeScript** (`conformance/runner/typescript/runner.test.ts` `TS_SDK_SKIPS`): - "Large integer IDs preserved without precision loss" — `Number` is 53-bit (waiver 1B.6). -**Kotlin** (`kotlin/conformance/.../Main.kt` — one tag-based branch; `KOTLIN_SKIPS` -is empty): -- "List operation returns first page with Link header" — skipped via the `link-header` tag branch, not `KOTLIN_SKIPS`: Kotlin auto-paginates by design, so a first-page-only requestCount assertion is inapplicable (architectural). +**Kotlin** (`kotlin/conformance/.../Main.kt` — `KOTLIN_SKIPS` is empty) — none +beyond the whole-case `link-header` tag branch described above. -**Swift** (`conformance/runner/swift/.../Runner.swift` — one tag-based branch; -`temporarySkips` is empty): -- "List operation returns first page with Link header" — skipped via the `link-header` tag branch, not `temporarySkips`: Swift auto-paginates by design, so a first-page-only requestCount assertion is inapplicable (architectural, identical to Kotlin and TypeScript). +**Swift** (`conformance/runner/swift/.../Runner.swift` — `temporarySkips` is +empty) — none beyond the whole-case `link-header` tag branch described above. Swift carries no capability skips. It is three-gate on retry (status, network, idempotent POST) and, since #563, retries the authenticated download hop, so diff --git a/conformance/runner/go/main.go b/conformance/runner/go/main.go index ad7c5512f3..d856547c45 100644 --- a/conformance/runner/go/main.go +++ b/conformance/runner/go/main.go @@ -1061,25 +1061,16 @@ func checkAssertion( ) *TestResult { sdkErr := opResult.err - // Detect if any mock response includes a Link header with rel="next". - // The real SDK auto-paginates, so actual requestCount will be >= expected. - hasLinkNextHeader := false - for _, mr := range tc.MockResponses { - if link, ok := mr.Headers["Link"]; ok && strings.Contains(link, `rel="next"`) { - hasLinkNextHeader = true - break - } - } - switch assertion.Type { case "requestCount": - expected := expectedInt(assertion.Expected) - if hasLinkNextHeader { - if requestCount < expected { - return fail(tc, fmt.Sprintf("Expected >= %d requests (SDK auto-paginates), got %d", expected, requestCount)) - } - } else if requestCount != expected { - return fail(tc, fmt.Sprintf("Expected %d requests, got %d", expected, requestCount)) + // The Go SDK auto-paginates list operations, so a fixture that counts + // first-page requests only is inapplicable — but ONLY its count is. + // The rest of the case still runs. See requestCountApplies (#573). + if !requestCountApplies(tc.Tags) { + return nil + } + if msg := checkRequestCount(requestCount, expectedInt(assertion.Expected)); msg != "" { + return fail(tc, msg) } case "delayBetweenRequests": diff --git a/conformance/runner/go/request_count.go b/conformance/runner/go/request_count.go new file mode 100644 index 0000000000..5964f0809c --- /dev/null +++ b/conformance/runner/go/request_count.go @@ -0,0 +1,60 @@ +package main + +import "fmt" + +// checkRequestCount validates one `requestCount` assertion, returning "" when +// it holds and a failure message otherwise. +// +// EXACT, always — including the auto-paginating fixtures. The runners used to +// relax this to a lower bound whenever any mock response carried a +// `Link: rel="next"` header, on the theory that an auto-paginating SDK would +// legitimately make more requests than the fixture named. That is backwards for +// the fixtures the relaxation covered: in conformance/tests/pagination.json, +// "Pagination stops at maxPages safety cap" and "maxItems caps results across +// pages" each queue THREE pages and expect TWO requests, because stopping early +// is the behavior under test. `>=` passes an SDK that ignored the cap and +// walked every page. "Auto-pagination follows Link headers across multiple +// pages" is the exposed case: its only assertions are requestCount and noError, +// so an over-fetch has nothing else to catch it — the other two happen to carry +// a `responseMeta` truncated assertion that fires instead, coverage by luck. +// +// The one fixture where the count genuinely does not apply to an +// auto-paginating SDK — "List operation returns first page with Link header", +// which asserts a single request — carries the `link-header` tag, and +// requestCountApplies reports false for it. Nothing that still reaches this +// function needs the relaxation. +// +// Swift took this in #558; #573 is the same fix for the other five runners. +func checkRequestCount(actual, expected int) string { + if actual != expected { + return fmt.Sprintf("Expected %d requests, got %d", expected, actual) + } + return "" +} + +// linkHeaderTag marks a fixture whose requestCount counts first-page requests +// only, which an auto-paginating SDK cannot satisfy. +const linkHeaderTag = "link-header" + +// requestCountApplies reports whether a fixture's `requestCount` assertion is +// meaningful for this SDK. +// +// SCOPE: this suppresses ONE ASSERTION, not the whole test case. An earlier +// revision skipped the entire `link-header` case in every runner, which took +// its `statusCode: 200` and `noError` assertions down with the inapplicable +// `requestCount` — Kotlin and Swift had always skipped the case wholesale, so +// once Go, Python, Ruby and TypeScript joined them the fixture was executed by +// nothing at all while still sitting in conformance/tests/pagination.json, +// passing conformance-fixtures-check and check-fixture-coverage. That is the +// #572 shape ("present, run by nothing") one layer down. Only the count is +// inapplicable; the status code and the absence of an error are not, and they +// are the assertions that catch an auto-paginating SDK that walked the Link +// header into an error. +func requestCountApplies(tags []string) bool { + for _, tag := range tags { + if tag == linkHeaderTag { + return false + } + } + return true +} diff --git a/conformance/runner/go/request_count_test.go b/conformance/runner/go/request_count_test.go new file mode 100644 index 0000000000..b4e2a5d29b --- /dev/null +++ b/conformance/runner/go/request_count_test.go @@ -0,0 +1,106 @@ +// Bounds contract for the requestCount assertion (#573). +// +// Until this commit the five non-Swift runners evaluated requestCount as a +// LOWER bound whenever any mock response carried `Link: rel="next"`. Every +// committed fixture passes under both rules, so nothing in the suite could tell +// them apart — the same shape as the #563 delayBetweenRequests regression these +// support modules exist to pin. The over-fetch case below is the one that +// distinguishes them, and it is the case that matters: pagination.json's +// maxPages and maxItems fixtures each queue three pages and assert two +// requests, so a lower bound green-passes an SDK that ignored the cap. + +package main + +import "testing" + +func TestRequestCountAcceptsTheExactCount(t *testing.T) { + if msg := checkRequestCount(2, 2); msg != "" { + t.Fatalf("exact match should pass; got %q", msg) + } +} + +func TestRequestCountRejectsAnUnderFetch(t *testing.T) { + if msg := checkRequestCount(1, 2); msg == "" { + t.Fatal("1 request where 2 were expected should fail") + } +} + +func TestRequestCountRejectsAnOverFetch(t *testing.T) { + // The regression. Under the old lower bound this returned "" — an SDK that + // walked all three queued pages instead of stopping at the maxPages cap + // reported a clean pass. + if msg := checkRequestCount(3, 2); msg == "" { + t.Fatal("3 requests where 2 were expected should fail; a lower bound would accept it") + } +} + +func TestRequestCountMessageNamesBothCounts(t *testing.T) { + msg := checkRequestCount(3, 2) + if msg != "Expected 2 requests, got 3" { + t.Fatalf("failure message should name expected and actual; got %q", msg) + } +} + +func TestRequestCountZeroRequestsIsNotAFreePass(t *testing.T) { + // A test whose operation never reached the wire records zero requests. + // That must fail an assertion expecting one, not read as "no data, no + // opinion". + if msg := checkRequestCount(0, 1); msg == "" { + t.Fatal("0 requests where 1 was expected should fail") + } +} + +func TestRequestCountZeroExpectedRequiresZeroActual(t *testing.T) { + if msg := checkRequestCount(0, 0); msg != "" { + t.Fatalf("0 expected and 0 actual should pass; got %q", msg) + } + if msg := checkRequestCount(1, 0); msg == "" { + t.Fatal("1 request where 0 were expected should fail") + } +} + +// Applicability contract (#573). The `link-header` fixture's requestCount is +// inapplicable to an auto-paginating SDK; its statusCode and noError +// assertions are not. Suppressing the CASE instead of the ASSERTION left the +// fixture executed by nothing at all — it stays in pagination.json and passes +// conformance-fixtures-check and check-fixture-coverage either way, so nothing +// else would have reported it. + +func TestRequestCountDoesNotApplyToLinkHeaderFixtures(t *testing.T) { + if requestCountApplies([]string{"pagination", "link-header"}) { + t.Fatal("link-header fixtures must not have their requestCount asserted") + } +} + +func TestRequestCountAppliesToEveryOtherFixture(t *testing.T) { + for _, tags := range [][]string{nil, {}, {"pagination"}, {"retry", "idempotent"}} { + if !requestCountApplies(tags) { + t.Fatalf("requestCount must be asserted for tags %v", tags) + } + } +} + +// The suppression is one assertion wide. If it ever grows to the whole case +// again, the fixture's other two assertions stop running everywhere they still +// run, and nothing downstream notices. +func TestLinkHeaderSuppressionIsScopedToTheCountAssertion(t *testing.T) { + tc := TestCase{ + Name: "List operation returns first page with Link header", + Tags: []string{"pagination", "link-header"}, + Assertions: []Assertion{ + {Type: "requestCount", Expected: float64(1)}, + {Type: "statusCode", Expected: float64(200)}, + {Type: "noError"}, + }, + } + live := 0 + for _, a := range tc.Assertions { + if a.Type == "requestCount" && !requestCountApplies(tc.Tags) { + continue + } + live++ + } + if live != 2 { + t.Fatalf("statusCode and noError must still be evaluated; %d assertion(s) live", live) + } +} diff --git a/conformance/runner/python/runner.py b/conformance/runner/python/runner.py index e8cb14ac04..6e07229a86 100644 --- a/conformance/runner/python/runner.py +++ b/conformance/runner/python/runner.py @@ -110,6 +110,57 @@ def check_delay_gaps( return None +def check_request_count(actual: int, expected: int) -> str | None: + """Validate one ``requestCount`` assertion; None when it holds. + + EXACT, always — including the auto-paginating fixtures. The runner used to + relax this to a lower bound whenever any mock response carried + ``Link: rel="next"``, on the theory that an auto-paginating SDK would + legitimately make more requests than the fixture named. That is backwards + for the fixtures the relaxation covered: in conformance/tests/pagination.json, + "Pagination stops at maxPages safety cap" and "maxItems caps results across + pages" each queue THREE pages and expect TWO requests, because stopping + early is the behavior under test. ``>=`` passes an SDK that ignored the cap + and walked every page. "Auto-pagination follows Link headers across multiple + pages" is the exposed case: its only assertions are requestCount and + noError, so an over-fetch has nothing else to catch it. + + The one fixture where the count genuinely does not apply to an + auto-paginating SDK — "List operation returns first page with Link header", + which asserts a single request — carries the ``link-header`` tag, and + ``request_count_applies`` reports False for it. Nothing that still reaches + this function needs the relaxation. + + Swift took this in #558; #573 is the same fix for the other five runners. + """ + if actual != expected: + return f"Expected {expected} requests, got {actual}" + return None + + +#: Marks a fixture whose requestCount counts first-page requests only, which an +#: auto-paginating SDK cannot satisfy. +LINK_HEADER_TAG = "link-header" + + +def request_count_applies(tags: list[str]) -> bool: + """Whether a fixture's ``requestCount`` assertion is meaningful for this SDK. + + SCOPE: this suppresses ONE ASSERTION, not the whole test case. An earlier + revision skipped the entire ``link-header`` case in every runner, which took + its ``statusCode: 200`` and ``noError`` assertions down with the + inapplicable ``requestCount`` — Kotlin and Swift had always skipped the case + wholesale, so once Go, Python, Ruby and TypeScript joined them the fixture + was executed by nothing at all while still sitting in + conformance/tests/pagination.json, passing conformance-fixtures-check and + check-fixture-coverage. That is the #572 shape ("present, run by nothing") + one layer down. Only the count is inapplicable; the status code and the + absence of an error are not, and they are the assertions that catch an + auto-paginating SDK that walked the Link header into an error. + """ + return LINK_HEADER_TAG not in tags + + @dataclass class TestTracker: requests: list[dict] = field(default_factory=list) @@ -564,13 +615,17 @@ def _verify_assertions(self, *, result: Any, error: Exception | None) -> TestRes for assertion in self._test.get("assertions", []): match assertion["type"]: case "requestCount": - actual = self._tracker.request_count - expected = assertion["expected"] - if self._auto_paginates(): - if actual < expected: - failures.append(f"Expected >= {expected} requests, got {actual}") - elif actual != expected: - failures.append(f"Expected {expected} requests, got {actual}") + # The Python SDK auto-paginates list operations, so a + # fixture that counts first-page requests only is + # inapplicable — but ONLY its count is. The rest of the + # case still runs. See request_count_applies (#573). + if not request_count_applies(self._test.get("tags", [])): + continue + failure = check_request_count( + self._tracker.request_count, assertion["expected"] + ) + if failure is not None: + failures.append(failure) case "delayBetweenRequests": # Not all gaps are retry gaps — the download flow's final diff --git a/conformance/runner/python/test_request_count.py b/conformance/runner/python/test_request_count.py new file mode 100644 index 0000000000..0127e4ad5a --- /dev/null +++ b/conformance/runner/python/test_request_count.py @@ -0,0 +1,77 @@ +"""Bounds contract for the requestCount assertion (#573). + +Until this commit the five non-Swift runners evaluated requestCount as a LOWER +bound whenever any mock response carried ``Link: rel="next"``. Every committed +fixture passes under both rules, so nothing in the suite could tell them apart — +the same shape as the #563 delayBetweenRequests regression these support modules +exist to pin. The over-fetch case below is the one that distinguishes them, and +it is the case that matters: pagination.json's maxPages and maxItems fixtures +each queue three pages and assert two requests, so a lower bound green-passes an +SDK that ignored the cap. +""" +from __future__ import annotations + +from runner import check_request_count, request_count_applies + + +def test_exact_count_passes(): + assert check_request_count(2, 2) is None + + +def test_under_fetch_fails(): + assert check_request_count(1, 2) is not None + + +def test_over_fetch_fails(): + # The regression. Under the old lower bound this returned None — an SDK + # that walked all three queued pages instead of stopping at the maxPages + # cap reported a clean pass. + assert check_request_count(3, 2) is not None + + +def test_failure_message_names_both_counts(): + assert check_request_count(3, 2) == "Expected 2 requests, got 3" + + +def test_zero_requests_is_not_a_free_pass(): + # A test whose operation never reached the wire records zero requests. + # That must fail an assertion expecting one, not read as "no data, no + # opinion". + assert check_request_count(0, 1) is not None + + +def test_zero_expected_requires_zero_actual(): + assert check_request_count(0, 0) is None + assert check_request_count(1, 0) is not None + + +# Applicability contract (#573). The ``link-header`` fixture's requestCount is +# inapplicable to an auto-paginating SDK; its statusCode and noError assertions +# are not. Suppressing the CASE instead of the ASSERTION left the fixture +# executed by nothing at all — it stays in pagination.json and passes +# conformance-fixtures-check and check-fixture-coverage either way, so nothing +# else would have reported it. + + +def test_request_count_does_not_apply_to_link_header_fixtures(): + assert not request_count_applies(["pagination", "link-header"]) + + +def test_request_count_applies_to_every_other_fixture(): + for tags in ([], ["pagination"], ["retry", "idempotent"]): + assert request_count_applies(tags) + + +def test_link_header_suppression_is_scoped_to_the_count_assertion(): + """The suppression is one assertion wide, not one case wide.""" + tags = ["pagination", "link-header"] + assertions = [ + {"type": "requestCount", "expected": 1}, + {"type": "statusCode", "expected": 200}, + {"type": "noError"}, + ] + live = [ + a for a in assertions + if not (a["type"] == "requestCount" and not request_count_applies(tags)) + ] + assert [a["type"] for a in live] == ["statusCode", "noError"] diff --git a/conformance/runner/ruby/request_count_test.rb b/conformance/runner/ruby/request_count_test.rb new file mode 100644 index 0000000000..e4a5e2dc99 --- /dev/null +++ b/conformance/runner/ruby/request_count_test.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +# Bounds contract for the requestCount assertion (#573). +# +# Until this commit the five non-Swift runners evaluated requestCount as a LOWER +# bound whenever any mock response carried `Link: rel="next"`. Every committed +# fixture passes under both rules, so nothing in the suite could tell them +# apart — the same shape as the #563 delayBetweenRequests regression these +# support modules exist to pin. The over-fetch case below is the one that +# distinguishes them, and it is the case that matters: pagination.json's +# maxPages and maxItems fixtures each queue three pages and assert two requests, +# so a lower bound green-passes an SDK that ignored the cap. +# +# Run: `bundle exec ruby request_count_test.rb` + +require "minitest/autorun" +require_relative "runner" + +class RequestCountTest < Minitest::Test + def test_exact_count_passes + assert_nil RequestCount.check(2, 2) + end + + def test_under_fetch_fails + refute_nil RequestCount.check(1, 2) + end + + def test_over_fetch_fails + # The regression. Under the old lower bound this returned nil — an SDK that + # walked all three queued pages instead of stopping at the maxPages cap + # reported a clean pass. + refute_nil RequestCount.check(3, 2) + end + + def test_failure_message_names_both_counts + assert_equal "Expected 2 requests, got 3", RequestCount.check(3, 2) + end + + def test_zero_requests_is_not_a_free_pass + # A test whose operation never reached the wire records zero requests. That + # must fail an assertion expecting one, not read as "no data, no opinion". + refute_nil RequestCount.check(0, 1) + end + + def test_zero_expected_requires_zero_actual + assert_nil RequestCount.check(0, 0) + refute_nil RequestCount.check(1, 0) + end + + # Applicability contract (#573). The `link-header` fixture's requestCount is + # inapplicable to an auto-paginating SDK; its statusCode and noError + # assertions are not. Suppressing the CASE instead of the ASSERTION left the + # fixture executed by nothing at all — it stays in pagination.json and passes + # conformance-fixtures-check and check-fixture-coverage either way, so + # nothing else would have reported it. + + def test_request_count_does_not_apply_to_link_header_fixtures + refute RequestCount.applies?(["pagination", "link-header"]) + end + + def test_request_count_applies_to_every_other_fixture + [nil, [], ["pagination"], ["retry", "idempotent"]].each do |tags| + assert RequestCount.applies?(tags), "requestCount must be asserted for #{tags.inspect}" + end + end + + # The suppression is one assertion wide, not one case wide. + def test_link_header_suppression_is_scoped_to_the_count_assertion + tags = ["pagination", "link-header"] + assertions = [ + { "type" => "requestCount", "expected" => 1 }, + { "type" => "statusCode", "expected" => 200 }, + { "type" => "noError" } + ] + live = assertions.reject do |a| + a["type"] == "requestCount" && !RequestCount.applies?(tags) + end + assert_equal ["statusCode", "noError"], live.map { |a| a["type"] } + end +end diff --git a/conformance/runner/ruby/runner.rb b/conformance/runner/ruby/runner.rb index 1619ac2a2f..d228069244 100644 --- a/conformance/runner/ruby/runner.rb +++ b/conformance/runner/ruby/runner.rb @@ -96,6 +96,57 @@ def self.named_gap_failure(delays, min_delay, index, request_count) end end +# The requestCount assertion contract, kept apart from the runner so its bounds +# branches are unit-testable (request_count_test.rb). +module RequestCount + # Validates one assertion, returning nil when it holds and a failure message + # otherwise. + # + # EXACT, always — including the auto-paginating fixtures. The runner used to + # relax this to a lower bound whenever any mock response carried + # `Link: rel="next"`, on the theory that an auto-paginating SDK would + # legitimately make more requests than the fixture named. That is backwards + # for the fixtures the relaxation covered: in conformance/tests/pagination.json, + # "Pagination stops at maxPages safety cap" and "maxItems caps results across + # pages" each queue THREE pages and expect TWO requests, because stopping + # early is the behavior under test. `>=` passes an SDK that ignored the cap + # and walked every page. "Auto-pagination follows Link headers across + # multiple pages" is the exposed case: its only assertions are requestCount + # and noError, so an over-fetch has nothing else to catch it. + # + # The one fixture where the count genuinely does not apply to an + # auto-paginating SDK — "List operation returns first page with Link header", + # which asserts a single request — carries the `link-header` tag, and + # `applies?` returns false for it. Nothing that still reaches this method + # needs the relaxation. + # + # Swift took this in #558; #573 is the same fix for the other five runners. + def self.check(actual, expected) + "Expected #{expected} requests, got #{actual}" unless actual == expected + end + + # Marks a fixture whose requestCount counts first-page requests only, which + # an auto-paginating SDK cannot satisfy. + LINK_HEADER_TAG = "link-header" + + # Whether a fixture's `requestCount` assertion is meaningful for this SDK. + # + # SCOPE: this suppresses ONE ASSERTION, not the whole test case. An earlier + # revision skipped the entire `link-header` case in every runner, which took + # its `statusCode: 200` and `noError` assertions down with the inapplicable + # `requestCount` — Kotlin and Swift had always skipped the case wholesale, so + # once Go, Python, Ruby and TypeScript joined them the fixture was executed by + # nothing at all while still sitting in conformance/tests/pagination.json, + # passing conformance-fixtures-check and check-fixture-coverage. That is the + # #572 shape ("present, run by nothing") one layer down. Only the count is + # inapplicable; the status code and the absence of an error are not, and they + # are the assertions that catch an auto-paginating SDK that walked the Link + # header into an error. + def self.applies?(tags) + !(tags || []).include?(LINK_HEADER_TAG) + end +end + # Test execution tracking class TestTracker attr_reader :requests @@ -656,16 +707,12 @@ def verify_assertions(result:, error:) (@test["assertions"] || []).each do |assertion| case assertion["type"] when "requestCount" - actual = @tracker.request_count - expected = assertion["expected"] - if auto_paginates? - unless actual >= expected - failures << "Expected >= #{expected} requests (SDK auto-paginates), got #{actual}" - end - else - unless actual == expected - failures << "Expected #{expected} requests, got #{actual}" - end + # The Ruby SDK auto-paginates list operations, so a fixture that counts + # first-page requests only is inapplicable — but ONLY its count is. The + # rest of the case still runs. See RequestCount.applies? (#573). + if RequestCount.applies?(@test["tags"]) + failure = RequestCount.check(@tracker.request_count, assertion["expected"]) + failures << failure if failure end when "delayBetweenRequests" diff --git a/conformance/runner/swift/Sources/ConformanceRunner/Assertions.swift b/conformance/runner/swift/Sources/ConformanceRunner/Assertions.swift index 23ac2dfcf5..86c67cb334 100644 --- a/conformance/runner/swift/Sources/ConformanceRunner/Assertions.swift +++ b/conformance/runner/swift/Sources/ConformanceRunner/Assertions.swift @@ -232,6 +232,16 @@ func evaluateAssertions( // every page. The first-page-only fixture, the one case where the // count genuinely does not apply to an auto-paginating SDK, is // excluded by its own `link-header` tag before it reaches here. + // + // Swift excludes the whole CASE where Go, Python, Ruby and + // TypeScript exclude only this ASSERTION (#573). Deliberate, not + // drift: `httpStatus` is the status of the last mock response the + // SDK consumed, and an auto-paginating SDK walks past the end of a + // one-response queue, so the fixture's `statusCode: 200` cannot be + // satisfied. Narrowing this arm was tried and reverted — `make + // conformance-swift` then reports `Expected status code 200, but + // got no response` and exits 2. Do not "align" it with the other + // four without widening the status model first. if requestCount != expected { return .fail("Expected \(expected) requests, got \(requestCount)") } diff --git a/conformance/runner/typescript/request-count.test.ts b/conformance/runner/typescript/request-count.test.ts new file mode 100644 index 0000000000..d404db3c2a --- /dev/null +++ b/conformance/runner/typescript/request-count.test.ts @@ -0,0 +1,81 @@ +/** + * Bounds contract for the requestCount assertion (#573). + * + * Until this commit the five non-Swift runners evaluated requestCount as a + * LOWER bound whenever any mock response carried `Link: rel="next"`. Every + * committed fixture passes under both rules, so nothing in the suite could tell + * them apart — the same shape as the #563 delayBetweenRequests regression these + * support modules exist to pin. The over-fetch case below is the one that + * distinguishes them, and it is the case that matters: pagination.json's + * maxPages and maxItems fixtures each queue three pages and assert two + * requests, so a lower bound green-passes an SDK that ignored the cap. + */ + +import { describe, expect, it } from "vitest"; +import { checkRequestCount, requestCountApplies } from "./request-count.js"; + +describe("checkRequestCount", () => { + it("passes on the exact count", () => { + expect(checkRequestCount(2, 2)).toBeUndefined(); + }); + + it("fails an under-fetch", () => { + expect(checkRequestCount(1, 2)).toBeDefined(); + }); + + it("fails an over-fetch", () => { + // The regression. Under the old lower bound this returned undefined — an + // SDK that walked all three queued pages instead of stopping at the + // maxPages cap reported a clean pass. + expect(checkRequestCount(3, 2)).toBeDefined(); + }); + + it("names both counts in the failure message", () => { + expect(checkRequestCount(3, 2)).toBe("Expected 2 requests, got 3"); + }); + + it("does not treat a zero-request run as a free pass", () => { + // A test whose operation never reached the wire records zero requests. + // That must fail an assertion expecting one, not read as "no data, no + // opinion". + expect(checkRequestCount(0, 1)).toBeDefined(); + }); + + it("requires zero actual when zero is expected", () => { + expect(checkRequestCount(0, 0)).toBeUndefined(); + expect(checkRequestCount(1, 0)).toBeDefined(); + }); +}); + +/** + * Applicability contract (#573). The `link-header` fixture's requestCount is + * inapplicable to an auto-paginating SDK; its statusCode and noError + * assertions are not. Suppressing the CASE instead of the ASSERTION left the + * fixture executed by nothing at all — it stays in pagination.json and passes + * conformance-fixtures-check and check-fixture-coverage either way, so nothing + * else would have reported it. + */ +describe("requestCountApplies", () => { + it("does not apply to link-header fixtures", () => { + expect(requestCountApplies(["pagination", "link-header"])).toBe(false); + }); + + it("applies to every other fixture", () => { + for (const tags of [undefined, [], ["pagination"], ["retry", "idempotent"]]) { + expect(requestCountApplies(tags)).toBe(true); + } + }); + + it("suppresses one assertion, not the whole case", () => { + const tags = ["pagination", "link-header"]; + const assertions = [ + { type: "requestCount", expected: 1 }, + { type: "statusCode", expected: 200 }, + { type: "noError" }, + ]; + const live = assertions.filter( + (a) => !(a.type === "requestCount" && !requestCountApplies(tags)), + ); + expect(live.map((a) => a.type)).toEqual(["statusCode", "noError"]); + }); +}); diff --git a/conformance/runner/typescript/request-count.ts b/conformance/runner/typescript/request-count.ts new file mode 100644 index 0000000000..a495f99733 --- /dev/null +++ b/conformance/runner/typescript/request-count.ts @@ -0,0 +1,58 @@ +/** + * The `requestCount` assertion contract, kept apart from the runner so its + * bounds branches are unit-testable (request-count.test.ts). + */ + +/** + * Validates one assertion, returning `undefined` when it holds and a failure + * message otherwise. + * + * EXACT, always — including the auto-paginating fixtures. The runner used to + * relax this to a lower bound whenever any mock response carried + * `Link: rel="next"`, on the theory that an auto-paginating SDK would + * legitimately make more requests than the fixture named. That is backwards for + * the fixtures the relaxation covered: in conformance/tests/pagination.json, + * "Pagination stops at maxPages safety cap" and "maxItems caps results across + * pages" each queue THREE pages and expect TWO requests, because stopping early + * is the behavior under test. `>=` passes an SDK that ignored the cap and + * walked every page. "Auto-pagination follows Link headers across multiple + * pages" is the exposed case: its only assertions are requestCount and noError, + * so an over-fetch has nothing else to catch it — the other two happen to carry + * a `responseMeta` truncated assertion that fires instead, coverage by luck. + * + * The one fixture where the count genuinely does not apply to an + * auto-paginating SDK — "List operation returns first page with Link header", + * which asserts a single request — carries the `link-header` tag, and + * `requestCountApplies` returns false for it. Nothing that still reaches this + * function needs the relaxation. + * + * Swift took this in #558; #573 is the same fix for the other five runners. + */ +export function checkRequestCount(actual: number, expected: number): string | undefined { + return actual === expected ? undefined : `Expected ${expected} requests, got ${actual}`; +} + +/** + * Marks a fixture whose requestCount counts first-page requests only, which an + * auto-paginating SDK cannot satisfy. + */ +export const LINK_HEADER_TAG = "link-header"; + +/** + * Whether a fixture's `requestCount` assertion is meaningful for this SDK. + * + * SCOPE: this suppresses ONE ASSERTION, not the whole test case. An earlier + * revision skipped the entire `link-header` case in every runner, which took + * its `statusCode: 200` and `noError` assertions down with the inapplicable + * `requestCount` — Kotlin and Swift had always skipped the case wholesale, so + * once Go, Python, Ruby and TypeScript joined them the fixture was executed by + * nothing at all while still sitting in conformance/tests/pagination.json, + * passing conformance-fixtures-check and check-fixture-coverage. That is the + * #572 shape ("present, run by nothing") one layer down. Only the count is + * inapplicable; the status code and the absence of an error are not, and they + * are the assertions that catch an auto-paginating SDK that walked the Link + * header into an error. + */ +export function requestCountApplies(tags: string[] | undefined): boolean { + return !(tags ?? []).includes(LINK_HEADER_TAG); +} diff --git a/conformance/runner/typescript/runner.test.ts b/conformance/runner/typescript/runner.test.ts index 2a7b029437..b38ff1ba63 100644 --- a/conformance/runner/typescript/runner.test.ts +++ b/conformance/runner/typescript/runner.test.ts @@ -17,6 +17,7 @@ import * as path from "node:path"; import { fileURLToPath } from "node:url"; import { checkDelayGaps } from "./delay-gaps.js"; import { errorRaisedFailure } from "./error-raised.js"; +import { checkRequestCount, requestCountApplies } from "./request-count.js"; // ============================================================================= // Types mirroring conformance/schema.json @@ -842,12 +843,6 @@ function checkAssertions( tracker: ReturnType, result: { error?: BasecampError | Error; httpStatus?: number; meta?: Record }, ): void { - // Detect if any mock response includes a Link header with rel="next". - // The TS SDK auto-paginates (follows all Link next headers), so the - // actual requestCount will be higher than what the conformance test - // expects. In this case, assert >= instead of strict equality. - const hasLinkNextHeader = suiteHasLinkNext(tc); - // DownloadURL implicit invariant: hop 1 must hit the test case path. // The MSW handler is origin-wide so hop 2's relative-resolved URL is // served, but a regression that misroutes hop 1 to a different path on @@ -895,19 +890,12 @@ function checkAssertions( for (const assertion of tc.assertions) { switch (assertion.type) { case "requestCount": { - const expected = Number(assertion.expected); - if (hasLinkNextHeader) { - // TS SDK auto-paginates: expect at least the specified count - expect( - tracker.requestCount(), - `[${tc.name}] expected >= ${expected} requests (SDK auto-paginates), got ${tracker.requestCount()}`, - ).toBeGreaterThanOrEqual(expected); - } else { - expect( - tracker.requestCount(), - `[${tc.name}] expected ${expected} requests, got ${tracker.requestCount()}`, - ).toBe(expected); - } + // The TS SDK auto-paginates list operations, so a fixture that counts + // first-page requests only is inapplicable — but ONLY its count is. + // The rest of the case still runs. See requestCountApplies (#573). + if (!requestCountApplies(tc.tags)) break; + const failure = checkRequestCount(tracker.requestCount(), Number(assertion.expected)); + if (failure) throw new Error(`[${tc.name}] ${failure}`); break; } diff --git a/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt b/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt index 03698233cb..95357831bd 100644 --- a/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt +++ b/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt @@ -347,13 +347,7 @@ private fun runTest(tc: TestCase): TestResult { "requestCount" -> { val expected = assertion.expected?.asInt() ?: return TestResult(false, "requestCount assertion missing expected value") - if (autoPaginates) { - if (requestCount < expected) { - return TestResult(false, "Expected >= $expected requests (SDK auto-paginates), got $requestCount") - } - } else if (requestCount != expected) { - return TestResult(false, "Expected $expected requests, got $requestCount") - } + checkRequestCount(requestCount, expected)?.let { return TestResult(false, it) } } "statusCode" -> { diff --git a/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/RequestCount.kt b/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/RequestCount.kt new file mode 100644 index 0000000000..2bedf6e268 --- /dev/null +++ b/kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/RequestCount.kt @@ -0,0 +1,43 @@ +package com.basecamp.sdk.conformance + +/** + * Validates one `requestCount` assertion, returning `null` when it holds and a + * failure message otherwise. + * + * EXACT, always — including the auto-paginating fixtures. The runner used to + * relax this to a lower bound whenever any mock response carried + * `Link: rel="next"`, on the theory that an auto-paginating SDK would + * legitimately make more requests than the fixture named. That is backwards for + * the fixtures the relaxation covered: in conformance/tests/pagination.json, + * "Pagination stops at maxPages safety cap" and "maxItems caps results across + * pages" each queue THREE pages and expect TWO requests, because stopping early + * is the behavior under test. `>=` passes an SDK that ignored the cap and + * walked every page. "Auto-pagination follows Link headers across multiple + * pages" is the exposed case: its only assertions are requestCount and noError, + * so an over-fetch has nothing else to catch it — the other two happen to carry + * a `responseMeta` truncated assertion that fires instead, coverage by luck. + * + * The one fixture where the count genuinely does not apply to an + * auto-paginating SDK — "List operation returns first page with Link header", + * which asserts a single request — carries the `link-header` tag and is already + * excluded by the runner before any assertion is evaluated. Nothing that still + * reaches this function needs the relaxation. + * + * Kotlin excludes the whole CASE where Go, Python, Ruby and TypeScript exclude + * only the requestCount ASSERTION (#573). That asymmetry is deliberate, not + * drift: `httpStatusCode` here is the status of the last MockEngine response + * the SDK consumed, and an auto-paginating SDK walks past the end of a + * one-response queue, so the fixture's `statusCode: 200` cannot be satisfied. + * Narrowing this arm to the assertion was tried and reverted — `make + * conformance-kotlin` then reports `Expected status code 200, but got no + * response` and exits 2. Widening the status model is separate work; until + * then, do not "align" this branch with the other four. + * + * The MockEngine's auto-pagination tolerance stays: answering an over-walk with + * a terminal empty page rather than an error is what lets this tightened + * assertion report a clean count mismatch instead of an opaque transport error. + * + * Swift took this in #558; #573 is the same fix for the other five runners. + */ +fun checkRequestCount(actual: Int, expected: Int): String? = + if (actual == expected) null else "Expected $expected requests, got $actual" diff --git a/kotlin/conformance/src/test/kotlin/com/basecamp/sdk/conformance/RequestCountTest.kt b/kotlin/conformance/src/test/kotlin/com/basecamp/sdk/conformance/RequestCountTest.kt new file mode 100644 index 0000000000..0da90264ec --- /dev/null +++ b/kotlin/conformance/src/test/kotlin/com/basecamp/sdk/conformance/RequestCountTest.kt @@ -0,0 +1,57 @@ +package com.basecamp.sdk.conformance + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertNull + +/** + * Bounds contract for the requestCount assertion (#573). + * + * Until this commit the five non-Swift runners evaluated requestCount as a + * LOWER bound whenever any mock response carried `Link: rel="next"`. Every + * committed fixture passes under both rules, so nothing in the suite could tell + * them apart — the same shape as the #563 delayBetweenRequests regression these + * support modules exist to pin. The over-fetch case below is the one that + * distinguishes them, and it is the case that matters: pagination.json's + * maxPages and maxItems fixtures each queue three pages and assert two + * requests, so a lower bound green-passes an SDK that ignored the cap. + */ +class RequestCountTest { + @Test + fun `the exact count passes`() { + assertNull(checkRequestCount(2, 2)) + } + + @Test + fun `an under-fetch fails`() { + assertNotNull(checkRequestCount(1, 2)) + } + + @Test + fun `an over-fetch fails`() { + // The regression. Under the old lower bound this returned null — an SDK + // that walked all three queued pages instead of stopping at the + // maxPages cap reported a clean pass. + assertNotNull(checkRequestCount(3, 2)) + } + + @Test + fun `the failure message names both counts`() { + assertEquals("Expected 2 requests, got 3", checkRequestCount(3, 2)) + } + + @Test + fun `a zero-request run is not a free pass`() { + // A test whose operation never reached the wire records zero requests. + // That must fail an assertion expecting one, not read as "no data, no + // opinion". + assertNotNull(checkRequestCount(0, 1)) + } + + @Test + fun `zero expected requires zero actual`() { + assertNull(checkRequestCount(0, 0)) + assertNotNull(checkRequestCount(1, 0)) + } +}