Skip to content

[ruby-compat] Follow up on the request cursor review - #129

Merged
chikamura merged 4 commits into
feature/ruby-compatibilityfrom
ruby-compat/test-infra-request-cursor-followup
Oct 15, 2026
Merged

chikamura merged 4 commits into
feature/ruby-compatibilityfrom
ruby-compat/test-infra-request-cursor-followup

Conversation

@chikamura

@chikamura chikamura commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #126, addressing the non-blocking suggestions in its review (#126 (review)). #126 itself stays at the approved commit; this PR is stacked on it and carries only the changes below. Test code only.

  • The cursor, not a count assertion, reports a diverging run. runWithMockServer() compared the request count to the response count before the test could walk a single request, so a step that gained or lost a call failed with a bare expected:<10> but was:<9> and the cursor never got to say where. The count check is gone: the cursor is handed every recorded request, a run that made fewer calls fails at the first next() with nothing left, and one that made more fails when the cursor is closed, whose message now lists the unasserted requests as GET /path (the 599 fail-fast answer usually fails the action before that). The harness self-tests cover both directions.
  • Completeness no longer depends on remembering a final call. RequestCursor is AutoCloseable and close() checks that every request was walked, so tests hold the cursor in a try-with-resources; the explicit assertNoMore() calls are gone. A failure in the try body still wins: the close() failure is attached to it as suppressed. The two harness self-tests for a run that made too many or too few requests keep the cursor outside a try on purpose, since they assert on the failure itself.
  • assertLoadAttempt() returns both upload requests. It returns a LoadAttempt holding the session-open POST and the file PUT, so testRunAppendDirectMode and its no-compression twin use it instead of re-implementing it inline, and a change to what one attempt consists of ([ruby-compat/07] Fail fast instead of swallowing data-loss errors #123) lands in one place. Callers that only want the POST read .openUpload off the result.
  • next() says which request the test asserts. The failure message read "after 9 ... only 9" (position printed twice); it now says "the test asserts request Retry for bq transfer #10 but the run recorded only 9", counting from 1 like the test's response list.
  • The re-wrapped runWithMockServer() comment no longer has a short line; it was rewritten with the first item.

Test plan

  • ./gradlew test (158 tests, 1 skipped)
  • ./gradlew spotlessApply

🤖 Generated with Claude Code

@chikamura chikamura changed the title Ruby compat/test infra request cursor followup [ruby-compat] Follow up on the request cursor review Oct 14, 2026
@chikamura
chikamura marked this pull request as ready for review October 14, 2026 02:56
@chikamura
chikamura requested a review from john-abre October 14, 2026 02:57

@john-abre john-abre left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve します。#126 で挙げた非ブロッキングの 6 点がすべて反映されています。
assertNoMore() の呼び出しは残っておらず、try-with-resources の外でカーソルを持つのは、失敗自体を検証するハーネスのセルフテスト 2 件だけであることを確認しました。
挙動も問題ありません: try 本体の失敗が優先されて close() の失敗は suppressed として付き、リクエストが少ない run は最初の next() で、多い run は close() が余分なリクエストを名指しして失敗します。セルフテストが両方向をカバーしています。
手元でも ./gradlew spotlessCheck test を実行し、158 テスト(1 skip)がすべて通りました。

🤖 Generated with Claude Code

@chikamura
chikamura force-pushed the ruby-compat/test-infra-request-cursor branch from 5fca3e9 to 0a3e564 Compare October 15, 2026 00:10
@chikamura
chikamura force-pushed the ruby-compat/test-infra-request-cursor-followup branch from 4e9f532 to f81c7ef Compare October 15, 2026 00:10
Base automatically changed from ruby-compat/test-infra-request-cursor to feature/ruby-compatibility October 15, 2026 00:11
chikamura and others added 4 commits October 15, 2026 09:12
runWithMockServer() compared the request count to the response count before
the test could walk a single request, so a step that gained or lost a call
failed with a bare "expected:<10> but was:<9>" and the cursor never got to
say where. The count check is gone: the cursor is handed every recorded
request, a run that made fewer calls fails at the first next() with nothing
left, and one that made more fails at assertNoMore(), whose message now lists
the unasserted requests as "GET /path" (the 599 fail-fast answer usually
fails the action before that). The harness self-tests cover both directions.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Completeness depended on each test remembering to end with
requests.assertNoMore(); a forgotten call would have silently brought back
the partial-assertion mode in which the strftime test skipped requests
(#112 / #119). RequestCursor is now AutoCloseable and close() does that
check, so tests hold the cursor in a try-with-resources and nothing is
left to remember. A failure in the try body still wins: the close() failure
is attached to it as suppressed.

The harness self-tests for a run that made too many or too few requests
keep the cursor outside a try on purpose, since they assert on the failure
itself (close() naming "GET /two", next() with nothing left).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
testRunAppendDirectMode and its no-compression twin re-implemented the
helper inline because it returned only the POST that opens the session,
and they also need the file PUT to check the uploaded bytes. The helper now
returns a LoadAttempt holding both, so those two tests use it as well and a
change to what one attempt consists of (#123) lands in one place. Callers
that only wanted the POST read .openUpload off the result.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
RequestCursor#next() printed position twice; at the point it fails
position equals the recorded count, so the message read "after 9 ... only
9". It now says which request (counting from 1, like the test's response
list) the test is asserting and how many were recorded.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@chikamura
chikamura force-pushed the ruby-compat/test-infra-request-cursor-followup branch from f81c7ef to 30ea515 Compare October 15, 2026 00:13
@chikamura
chikamura merged commit 35acdc7 into feature/ruby-compatibility Oct 15, 2026
@chikamura
chikamura deleted the ruby-compat/test-infra-request-cursor-followup branch October 15, 2026 00: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.

2 participants