Repository navigation
Conversation
readRequestBodyAsString() and uploadedLines() drained the RecordedRequest's Buffer, so a second read of the same request (jobIdOf() then jobConfig() on the session-open POST, or uploadedLines() twice on the same PUT) saw an empty body and failed in the JSON parser or GZIPInputStream instead of pointing at the harness. Both now read getBody().clone(); a harness self-test pins it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… delete it The class re-creates private internals of google-cloud-bigquery 2.14.0 (the request factory, the resumable upload path, the upstream header typo), so a library bump could change its behavior without breaking compilation. Record that the version pin has no other reason, that the upstream fix ships in 2.26.1, and that the testhost seam goes away once the library is upgraded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The resumable upload's Location was hand-built from server.getHostName() and
server.getPort() while the client's test_host came from server.url("/"). Had
the two ever disagreed (TLS, an IPv6 host literal), the chunk PUTs would have
gone to a different origin than the rest of the calls and failed with an
obscure connection error. Both now come from testHostOf(), which returns the
origin the client is pointed at.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The self-test asserted JUnit's "expected:<1> but was:<2>" wording, so a JUnit upgrade or a change to the count assertion would have failed it without the harness being broken. Give the count assertion its own message and match on that instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
spotless had split the inline comments in testRunReplaceModeStillUpdatesSchemaWhenTempTableDeleteFails across lines, leaving a ")" floating and "delete temp table (finally)" attached to toArray() instead of the errorResponse() it describes. They now sit on their own line above the response they explain, like the other tests' comments. The note on how a load attempt differs from copy/query (3 requests per attempt, each retry a new job) was on newLoadFile(), a helper that just writes a one-line file; it moves to the first retry test it describes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
chikamura
deleted the
ruby-compat/test-infra-upload-over-http-followup
branch
October 14, 2026 01:54
2 tasks done
chikamura
added a commit
that referenced
this pull request
Oct 14, 2026
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>
2 tasks done
Contributor
Author
|
This merge has been undone together with #125's: both landed before the review on #125 had been addressed and approved, |
chikamura
added a commit
that referenced
this pull request
Oct 15, 2026
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
added a commit
that referenced
this pull request
Oct 15, 2026
… index (#126) * Assert recorded requests through a cursor instead of by index 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> * Walk the merged no-compression test with the request cursor 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> * Return a RequestCursor from every runWithMockServer() 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> * Adapt the harness self-test to runWithMockServer() returning a cursor 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> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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
1 participant
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
Follow-up to #125, which was squash-merged before the review there (#125 (review)) was addressed. This PR carries the fixes for every point raised, including the one marked as needed before merge. Test code and comments only; no production behavior changes.
readRequestBodyAsString()anduploadedLines()drained theRecordedRequest'sBuffer, so a second read of the same request (e.g.jobIdOf()thenjobConfig()on the session-open POST) saw an empty body and failed in the JSON parser orGZIPInputStream. Both now readgetBody().clone(); a harness self-test pins it.Locationwas hand-built fromgetHostName()/getPort()whiletest_hostcame fromserver.url("/"). Both now come fromtestHostOf().runWithMockServer()'s request-count assertion has its own message (unexpected request count) and the self-test matches on that.TestHostBigQueryRpcrecords why it is tied to google-cloud-bigquery 2.14.0 (no reason for the pin other than not having been bumped), that the upstream fix ships in 2.26.1, that thetesthostseam goes away once the library is upgraded, and what to check on any bump until then.testRunReplaceModeStillUpdatesSchemaWhenTempTableDeleteFails), and the load-attempt retry note moved fromnewLoadFile()to the test it describes.The remaining review premise, that #123 covers both the swallowed upload exception and the channel
close()finalizing a partial file, is handled in #123 itself (its description will be updated to say so explicitly).Test plan
./gradlew test(157 tests, 1 skipped)./gradlew spotlessApply🤖 Generated with Claude Code