Repository navigation
[ruby-compat] Assert recorded requests through a cursor instead of by index - #126
Merged
chikamura merged 4 commits intoOct 15, 2026
Merged
Conversation
2 tasks done
john-abre
approved these changes
Oct 13, 2026
There was a problem hiding this comment.
Approve します。テストのみの変更で、呼び出し箇所はすべて一貫して移行されており、各カーソルは(直接または assertJobRetryRequests 経由で)assertNoMore() で終わっています。正確性に関わる問題は見つかりませんでした。
以下は非ブロッキングの提案です。フォローアップ PR に含めていただければ十分です。
runWithMockServer()が冒頭でリクエスト数をそのまま assert している(BigqueryMockWebServerTestUtil.java:93)。あるステップでリクエストが増減すると、requests.next()が一度も走る前にexpected:<10> but was:<9>という素の失敗で止まってしまい、カーソルが失敗箇所を特定する機会がありません。599 の fail-fast dispatcher が「リクエスト過多」のケースをすでにカバーしているので、件数 assert を外してnext()/assertNoMore()に最初に食い違ったステップを報告させる形を検討してください。assertNoMore()が件数しか報告しない(:316)。requests.subList(position, size)のメソッドとパスをメッセージに含めると、この assert が捕まえるべき失敗がそのまま読めるようになります。- 網羅性が
assertNoMore()を呼び忘れないことに依存している。呼び忘れると、strftime テストでリクエストがスキップされていた部分 assert モード(#112 / #119)に静かに戻ってしまいます。RequestCursorをAutoCloseableにしてclose()でassertNoMore()を呼ぶか、runWithMockServer()に assert ブロックを渡す形にすると、構造的に強制できます。 testRunAppendDirectMode/...WithoutCompressionがassertLoadAttempt()をインラインで再実装している(TestBigqueryJavaOutputPluginWithMockServer.java:131,:162)。ヘルパーがセッション開始の POST しか返さず、ファイル PUT を返さないためです。両方を返す(openUpload/uploadFileを持つ小さなホルダーなど)ようにすれば、これらのテストもヘルパーを使えるようになり、#123 で 3 リクエスト構成が変わる際の修正箇所も一か所に収まります。RequestCursor.next()の失敗メッセージでpositionが 2 回出力されている(:309)。2 つ目のプレースホルダーはrequests.size()のつもりだったと思われます。「テストは #10 のリクエストを assert しているが、記録されたのは 9 件のみ」のようなメッセージにすると、responses リストに直接目が向きます。- 再折り返しされた
runWithMockServer()のコメントに短い行が残っている(:66): "is expected to make exactly / responses.length HTTP / calls against the server"。
PR をまたぐ補足: uploadedLines() と readRequestBodyAsString() は RecordedRequest のボディを最初の読み取りで消費してしまいます(#125 で指摘済み)。カーソルによって同じリクエストを 2 回 assert しやすくなるので、その修正をこの PR のマージ前に入れておく価値があります。
🤖 Generated with Claude Code
2 tasks done
Base automatically changed from
ruby-compat/test-infra-upload-over-http
to
feature/ruby-compatibility
October 14, 2026 01:41
chikamura
force-pushed
the
feature/ruby-compatibility
branch
from
October 14, 2026 02:14
6301893 to
1a796dd
Compare
chikamura
force-pushed
the
ruby-compat/test-infra-request-cursor
branch
from
October 14, 2026 02:24
e223d85 to
db183ea
Compare
2 tasks done
chikamura
changed the base branch from
feature/ruby-compatibility
to
ruby-compat/test-infra-upload-over-http-v2
October 14, 2026 02:31
chikamura
force-pushed
the
ruby-compat/test-infra-request-cursor
branch
from
October 14, 2026 02:38
db183ea to
5fca3e9
Compare
2 tasks done
Base automatically changed from
ruby-compat/test-infra-upload-over-http-v2
to
feature/ruby-compatibility
October 15, 2026 00:02
The e2e MockWebServer tests addressed requests as requests.get(n), so every step that gained or lost a request forced renumbering of everything after it; that bit when #112 and #119 were merged and again when the load path became two requests. Add RequestCursor (next() / assertNoMore()) and walk the requests in order. Requests referenced twice get a named variable (createTempTable, openUpload, ...). testRunExpandsStrftimeInTableName now also asserts the three requests it used to skip, since a cursor can't jump over them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The test came in from the upload-over-http branch in the positional form, which no longer compiles here since the e2e wrappers return a RequestCursor. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The client-level tests still addressed requests by position; now every runWithMockServer*() hands back a RequestCursor and all MockWebServer tests walk it, so no test depends on request indices any more. The List + offset assertLoadAttempt() goes away; the cursor one returns the POST that opened the upload session, so a test that needs its body (job id, load config) takes it from the return value instead of expanding the three asserts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The body-reread self-test from #127 took the first request with get(0) on the list runWithMockServer() used to return; with the cursor it is next(). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
chikamura
force-pushed
the
ruby-compat/test-infra-request-cursor
branch
from
October 15, 2026 00:10
5fca3e9 to
0a3e564
Compare
10 of 21 tasks
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
The e2e MockWebServer tests addressed the recorded requests by position (
requests.get(n)), so every step that gained or lost a request forced renumbering of everythingafter it. That bit twice already: when #112 (schema update moved before the temp table delete) and #119 were merged, the strftime test broke on CI, and #125 (load as a
session-open
POST+ filePUT) had to renumber all 10 load tests.RequestCursortoBigqueryMockWebServerTestUtil:next()hands out the recorded requests in order andassertNoMore()checks that every one of them was assertedon. The e2e wrappers
runWithMockServer()/runWithMockServerExpectingFailure()now return it.TestBigqueryJavaOutputPluginWithMockServeras a sequence ofassertXxx(requests.next())calls ending inrequests.assertNoMore(). A requestthat a test needs twice (for its body, e.g. the temp table id or the load/copy job config) gets a named variable:
createTempTable,createTable,openUpload,uploadFile,createJob,patchRequest.assertLoadAttempt(RequestCursor, jobId)consumes the 3 requests of one load attempt; theList+ offset overload stays forTestBigqueryClientWithMockServer, whichis unchanged.
testRunExpandsStrftimeInTableNamenow also asserts the three requests it used to skip over (GET dataset, the temp table row count, the copy job status), since a cursorcan't jump over them. No other test changes what it verifies.
Test plan
./gradlew test(122 tests)./gradlew spotlessApplyNotes
ruby-compat/test-infra-upload-over-http); the base will be switched tofeature/ruby-compatibilityonce [ruby-compat] Exercise the real resumable upload in MockWebServer tests #125 is merged.🤖 Generated with Claude Code