Repository navigation
Conversation
…ugin (#110) * Add test_host seam for MockWebServer-based BigQuery API tests Adds a test-only test_host config option that redirects BigqueryClient at a local MockWebServer instead of the real BigQuery API and bypasses credential setup. BigqueryClient#load() also routes load jobs through jobs.insert in this mode, since TableDataWriteChannel's resumable upload session hardcodes the real BigQuery host regardless of BigQueryOptions#getHost() (a google-cloud-bigquery:2.14.0 limitation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Harden BigqueryTestHostSupport and gate it behind TEST_HOST_ENABLED Adds bounded RetrySettings for the test-host BigQuery client so retry tests don't wait out the library's real default backoff, refactors getBigQueryService() to apply setProjectId() once regardless of branch, and requires the TEST_HOST_ENABLED environment variable (set only by the `test` Gradle task) in addition to the test_host config, so a plugin config alone can no longer redirect a real embulk run at an arbitrary unauthenticated host. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Add MockWebServer-based test suite for BigqueryClient and BigqueryJavaOutputPlugin Introduces a shared MockWebServer test harness (BigqueryMockWebServerTestUtil, AssertUtil) and builds out coverage for BigqueryClient (retry behavior for load/copy/query/createTableIfNotExist, updateTableIfNeed's interaction with mode/retain flags/column_options) and BigqueryJavaOutputPlugin (an end-to-end run per mode against a mocked BigQuery API). Also consolidates duplicated request/response fixtures into the shared util as they emerged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Move merge_keys default out of test_host.yml into the merge-mode test merge_keys was only ever needed to keep testRunMergeModeWithMockWebServer from triggering the INFORMATION_SCHEMA merge-key lookup; set it there explicitly instead of baking it into every test's default config. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Exercise the job-result-check path in MockWebServer plugin tests is_skip_job_result_check defaulted to true in test_host.yml, so no MockWebServer test ever exercised getTransactionReport()'s row-count check or the temp table row-count lookup it performs. Flip it to false (matching the plugin's own production default) and update the affected tests: add outputRows to load job responses, enqueue the extra temp table GET, and shift request indices accordingly. Also clear BigqueryUtil's static FileWriterHolder map between tests (test-only: a real embulk run never calls transaction() twice in one JVM, but JUnit reuses the JVM/threads across test methods, which was inflating num_input_rows with stale writers from earlier tests). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Split MockWebServer-based tests into their own test classes Moves the MockWebServer-based tests out of TestBigqueryClient and TestBigqueryJavaOutputPlugin into new TestBigqueryClientWithMockServer and TestBigqueryJavaOutputPluginWithMockServer classes, leaving the original classes containing only their pre-existing, non-MockServer tests. Drops the now-redundant "WithMockWebServer" suffix from individual test method names, since the class name already says so, and updates a README reference to the class that used to own this behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Note the TEST_HOST_ENABLED requirement on getTestHost() Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Link the google-cloud-bigquery source hardcoding the resumable-upload host Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Add delete_in_advance test for a missing destination table First run against a fresh dataset: the destination table doesn't exist, so autoCreate()'s tables.delete gets a 404. The client library maps that to BigQuery#delete() returning false, and the plugin ignores the return value, so the run proceeds exactly as when the delete succeeds. Pin that down with a mock-server run that returns 404 on the initial delete. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: John Abrehamson <john.abrehamson@primenumber.com>
… schema update (#112) * Guarantee temp table cleanup and fix its ordering vs schema update Ruby's begin/ensure wraps everything from right after auto_create through the final schema patch, and always deletes the temp table (and cleans up local files) afterward, so cleanup happens even if the load, copy/merge, or schema update fails. The java port only wrapped the copy/merge step, deleted the temp table inline before the schema update, and skipped it if a delete came before an unrelated failure. Wrap the whole post-autoCreate body in try/finally so a failure anywhere (storeCachedSrcFieldsIfNeed, load, copy/merge, updateTableIfNeed) still guarantees the temp table delete, nest a second finally for the local intermediate file cleanup so a delete failure doesn't block it either, and move updateTableIfNeed() before the temp table delete so a delete failure no longer blocks the schema update from being attempted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Test that the temp table is deleted even when paths is empty transaction() returns right after creating the destination table when paths.isEmpty(), before ever reaching the load/copy/updateTableIfNeed logic. Add a regression test covering that the temp table (already created by autoCreate()) still gets deleted via the guaranteed finally block in that case, using zero input records to reproduce paths.isEmpty() (BigqueryPageOutput#add() is never called, so no writer is registered and no intermediate file is ever created). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Align the missing-destination-table delete_in_advance test with the reordered cleanup That test was written against the previous end-of-run order (delete the temp table, then updateTableIfNeed()'s GET). This branch swaps those two steps, so its mock responses were being served to the wrong requests: the GET got a body-less 204 and updateTableIfNeed() NPE'd. Mirror testRunDeleteInAdvanceMode's order, keeping the initial 404 as the only difference between the two. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2 tasks done
json_column_visitor's else branch put the raw JSON string as a String
value, which BigQuery's JSON loader re-encodes as a quoted string
(e.g. {"msg":"hello"} became "{\"msg\":\"hello\"}"). Set the parsed
value directly for type: JSON instead, matching ruby's json_converter
which just passes the value through unchanged for that type.
Also document type: JSON as a supported column_options conversion
target for json columns in README and BigqueryColumnOption, since it
was previously implemented in ruby but undocumented in both projects.
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
#117) * Match ruby's timestamp-to-INTEGER/FLOAT/STRING output INTEGER/FLOAT now output epoch seconds instead of milliseconds, and default_timestamp_format no longer appends a timezone offset, matching ruby's timestamp_converter and DEFAULT_TIMESTAMP_FORMAT. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Add tests pinning ruby's timestamp conversion and default_timezone behavior Cover the review feedback on the timestamp conversion change: - INTEGER: sub-second values are truncated (999ms -> same second) and pre-1970 values are floored (-1ms -> -1), matching ruby's Time#to_i. - FLOAT: the fraction case now asserts an exact double, plus pre-1970 cases (-500ms -> -0.5, -1ms -> -0.001 as ruby's Time#to_f rounds once). - default_timezone: timestamp -> STRING/DATE/DATETIME, string -> TIMESTAMP, and nested RECORD TIMESTAMP fields fall back to task default_timezone when the column/field has no timezone, as ruby's `timezone || default_timezone` does. The -1ms FLOAT case and the default_timezone cases fail until the converter is updated; the rest already pass and only guard the behavior. TestBigqueryRecordConverter drops its @before fixture for a createTask() helper so a test can build a task with a different default_timezone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Fall back to default_timezone and round FLOAT epoch once, matching ruby column_options[].timezone and fields[].timezone defaulted to "UTC" on the option itself, so task-level default_timezone was never read and timestamp -> STRING/DATE/DATETIME (and string/nested-field parsing) ignored it. Make timezone Optional and resolve `timezone || default_timezone` at every use, as ruby's ValueConverterFactory does. FLOAT conversion added epoch seconds and a nanosecond fraction as doubles, which rounds twice and differs from ruby's Time#to_f near the epoch (-1ms gave -0.0010000000000000009). Sum via BigDecimal and round once. README gains an upgrade note: how to restore the old offset-bearing STRING default, and that INTEGER/FLOAT milliseconds can't be restored. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Consolidate INTEGER/FLOAT timestamp conversion tests The base INTEGER test now uses a sub-second value so it pins truncation on its own, replacing the separate truncation test; the pre-1970 floor test moves next to it. For FLOAT, the -1ms case already covers a negative value, so it takes over the beforeEpoch name and the redundant -500ms case is dropped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…#119) * Add strftime expansion for table names Ruby's configure() applies now.strftime(table) to the table name, letting configs like table_%Y%m%d generate date-based table names. Add the same expansion in BigqueryTaskBuilder using embulk-util-timestamp's ruby-compatible formatter, with an overload that takes an explicit ZoneId/Instant for deterministic testing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Add MockWebServer test for strftime-expanded table names Confirm end-to-end that a table name like table_%Y%m%d, expanded by BigqueryTaskBuilder before any BigQuery request is made, is what actually reaches the load/copy/get requests recorded against the mocked BigQuery API. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Expand strftime in old_table to match ruby old_table is only consumed by replace_backup, which isn't supported yet, but ruby expands strftime in both table and table_old unconditionally, so mirror that here to stay ready for when replace_backup is added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Document table id strftime formatting in README Ruby documents this under "Table id formatting", linking to ruby's Time#strftime docs; add the java equivalent, pointing out that the legacy Embulk-style formatter (embulk-util-timestamp) reimplements ruby's strftime rather than the C library's, and keep the link to strftime support separate from the pre-existing (and only partially accurate) partition decorator note in the same table row. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Sample the clock once when expanding table and old_table Expanding table and old_table with separate Instant.now() calls could stamp different timestamps into the two names (e.g. %S/%N patterns or a run crossing midnight). Take now/zone once in build() and pass them to both expansions, as ruby does; drop the null-defaulting overload that made this easy to get wrong. Replace the helper-only old_table tests with a build()-level test that goes through the getOldTable() branch, and trim redundant expandStrftime tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Assert table and old_table share the same timestamp suffix The test only checked that each name ends in 23 digits, so build() sampling the clock twice would still have passed. Compare the two suffixes directly to pin the single-sample behavior the test is named for. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Import java.time types in TestBigqueryTaskBuilder Instant and ZoneId were spelled out with fully qualified names inline, unlike every other type in the file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Pin strftime expansion to the system default time zone README says table names expand in the embulk server's local time, but no test exercised a zone other than UTC, so switching build() to UTC or default_timezone would have gone unnoticed. Run build() with the JVM default set to Asia/Tokyo and check the hour follows it. TimeZone.setDefault is JVM-global; the test notes that it relies on the suite running sequentially within a JVM. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
testRunExpandsStrftimeInTableName (#119) was written against a base that predates #112, which moved updateTableIfNeed() before the temp table delete. After the merge the GET table for updateTableIfNeed() received the empty DELETE response and getTable() returned null (NPE). Order the responses and request-index assertions like testRunReplaceMode. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…all, not just jobs (#113) * Apply retries/timeout config to every BigQuery API call, not just jobs Previously the retries/open_timeout_sec/read_timeout_sec config only affected load/copy/query job polling; other BigQuery API calls (get, create, update, delete on datasets and tables) fell back to the client library's own defaults. Wire retries into RetrySettings' maxAttempts and open_timeout_sec/read_timeout_sec into HttpTransportOptions so they apply uniformly to every API call. Also accept send_timeout_sec as a config option purely for compatibility with the ruby plugin's config surface. The underlying HTTP transport has no timeout for the request-send phase (only connect and read), so there is no direct java equivalent; its value is folded into read_timeout_sec instead of being dropped. See README.md for details and finer retry-backoff tuning available via environment variables. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Lift gax's 50s retry total timeout so retries is the effective limit ServiceOptions.getDefaultRetrySettings() carries totalTimeout = 50s, and gax stops retrying once the elapsed time plus the next delay would exceed it. With the default backoff that caps `retries` at about 6 attempts, and a single long read timeout leaves no room for any retry at all, so the configured value was never honored. Ruby's client retries by count with no wall-clock cap; set the total timeout to one day so maxAttempts is the effective limit, keeping the env var override for tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Accept deprecated timeout_sec as a fallback for read_timeout_sec Ruby still reads timeout_sec as `read_timeout_sec || timeout_sec || 300` and warns that it is deprecated. Java had no such key, and embulk's config mapper ignores unknown keys, so a migrated config with timeout_sec silently fell back to the 300s default. Resolve the read timeout the same way ruby does and log the same deprecation warning. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Expose retry backoff as plugin config instead of environment variables The gax retry backoff (initial/max delay, multiplier, total timeout) and the job-level retry waits were tunable only through BIGQUERY_OUTPUT_OPTION_* environment variables, parsed on every call with no variable name in the error, and the fast backoff for tests lived in build.gradle so it only applied under `./gradlew test`. Make them regular plugin options with explicit defaults in seconds, matching the existing *_timeout_sec keys: retry_initial_delay_sec (1), retry_max_delay_sec (32) and retry_delay_multiplier (2.0) mirror gax's defaults, retry_total_timeout_sec keeps the one-day cap, and job_retry_initial_wait_sec (2) / job_retry_max_wait_sec (10) keep the values that were hardcoded before. Tests set them in test_host.yml. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Validate retry and timeout ranges up front with ConfigException The *_sec options are converted to int milliseconds for HttpTransportOptions and RetryExecutor, which silently treat a negative or overflowed value as "unset" (20s) or sleep on it unchecked, and gax rejects out-of-range retry settings only at client construction with its own IllegalStateException. Check them in BigqueryConfigValidator instead, so a bad value fails at configure time with the offending key and bound in the message: - retries must be >= 0 and below Integer.MAX_VALUE (maxAttempts is +1) - retry/job-retry delays and every timeout must fit in int milliseconds, including the folded read_timeout_sec + send_timeout_sec that the transport actually receives (BigqueryClient#effectiveReadTimeoutSec) - retry_delay_multiplier >= 1.0 and retry_max_delay_sec >= initial The deprecation warning for timeout_sec moves there as well, so it is logged once at configure time. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Give MockWebServer tests a generous retry total timeout gax stops retrying once elapsed time plus the next delay would exceed the total timeout, so a 1s budget could cut the retry short on a slow run and make the retry-then-succeed tests flaky. The backoff delays are already zero, so the budget doesn't affect test time; raise it to 60s. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Move read timeout resolution into config.BigqueryConfigResolver The validator's folded read+send range check needed the same `read_timeout_sec || timeout_sec || 300` resolution the client uses, which made the config package reach back into BigqueryClient. Give the resolution its own small class in the config package so the dependency runs one way, and cover it directly (plus the deprecated-alias path of the sum check). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Drop the timeout_sec deprecation warning The deprecated alias is still resolved as a fallback for read_timeout_sec, but a run shouldn't log a warning on every start just because an existing config carries the key; keep the fallback silent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Reject NaN for retry_delay_multiplier NaN compares false against every bound, so it slipped through both this validator's `< 1.0` check and gax's own, leaving the backoff undefined. Reject it explicitly alongside values below 1.0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Disable gax retry jitter and correct the ruby wall-clock cap notes gax draws each retry delay uniformly from [0, delay] when jittered, which halves the expected wait compared to the ruby plugin, whose HTTP client library randomizes around the nominal value (0.5x to 1.5x). Applying the configured delays as-is keeps the java backoff inside ruby's range. README and PluginTask claimed ruby's retries have no wall-clock cap; its client library in fact applies a fixed 15-minute cap. The 1-day default for retry_total_timeout_sec stays, with the rationale reworded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Share the config test fixture between the resolver and validator tests TestBigqueryConfigResolver re-implemented the ConfigMapper setup, resource path and taskWith() helper that TestBigqueryConfigValidator already had. Move them into a BigqueryConfigTestBase that both tests extend. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Document that a timeout of 0 means no timeout open_timeout_sec and read_timeout_sec accept 0, which HttpTransportOptions passes through as "wait forever". That matches the ruby plugin, whose httpclient also treats 0 as no timeout, so keep accepting it and say so in README and the validator. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
chikamura
force-pushed
the
feature/ruby-compatibility
branch
from
October 14, 2026 02:14
6301893 to
1a796dd
Compare
…ts (#128) * Exercise the real resumable upload in MockWebServer tests google-cloud-bigquery 2.14.0 hardcodes the googleapis.com host for the request that opens a resumable upload session (HttpBigQueryRpc#open()), so BigqueryClient#load() used to take a test-only jobs.insert path under test_host, leaving the actual upload code untested. Add TestHostBigQueryRpc, an HttpBigQueryRpc subclass that builds that one URL from the configured host (what upstream does since googleapis/java-bigquery#2696), install it through the test_host BigQueryOptions, and drop the jobs.insert bypass. The two test-host classes move to their own package (testhost.TestHostBigQueryOptions / Rpc). The MockWebServer tests now see the real session-open POST and file PUT and assert on the uploaded bytes as well. loadAttemptResponses() / assertLoadAttempt() describe one load attempt (open, PUT, jobs.get) so retry scenarios list attempts instead of individual responses, and responses() flattens those arrays among single responses. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Tighten the MockWebServer load helpers after review - Replace the Object-varargs responses() with the type-safe MockResponses builder (of/add/loadAttempt/toArray). - assertUploadLoadFile() now states that it models a single-chunk upload and fails with a clear message for files over the library's chunk size. - uploadedLines() detects gzip by its magic bytes instead of assuming it. - Update comments that still described the old jobs.insert load path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Drop MockResponses#loadAttempt() in favor of add(loadAttemptResponses()) add(MockResponse...) already takes the array, so the shortcut only duplicated loadAttemptResponses(). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Make uploadedLines() take the expected compression instead of sniffing it Auto-detecting gzip would quietly accept a plain upload under compression: GZIP; stating the expectation turns it into a check. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Fail fast on unanswered mock requests and shorten test HTTP timeouts A request for which no response was enqueued used to block until the client's read timeout (300 + 300 s with #113's defaults), so a test that listed one response too few hung for 10 minutes before failing. The MockWebServer now answers such requests immediately with a 599, and test_host.yml sets 1 s open/read timeouts so a server that deliberately stays silent (SocketPolicy.NO_RESPONSE) fails the test in a second as well. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Add MockWebServer helpers for upload failures, chunked uploads and job ids Groundwork for tests of BigqueryClient#load()'s failure handling at the HTTP level: startResumableUploadResponse() is public and loadAttemptResponses() is split into uploadLoadFileResponse() / waitForLoadJobResponse(), so a test can put failing PUTs between the session open and the final chunk; disconnectDuringUploadResponse() is the one socket policy the library sees as a retryable network failure; uploadChunkAcceptedResponse(), assertUploadChunk() and writeLoadFile() cover files over the 15 MB chunk size; jobIdOf() reads the job id from the session-open POST. Tests pin what the library does on its own today: a 5xx or a connection reset during a chunk PUT is re-sent `retries` times on the same session, a 4xx is not, a 16 MB file goes up as two chunks, and a job that failed is retried under a new job id. An e2e run with compression: NONE checks the plain upload body. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Test the read timeout on uploads and the harness's fail-fast answer - unansweredResponse() (SocketPolicy.NO_RESPONSE) and a test that a chunk PUT nobody answers fails after read_timeout_sec, retried once like any network failure: the timeout from #113 applies to the upload too. Costs about 2 s of real waiting. - A test of the harness itself: a request beyond the enqueued responses is answered with a 599 at once, and the run is reported as having made more requests than expected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Have assertUploadLoadFile() delegate to assertUploadChunk() Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Read a clone of the recorded request body so it can be asserted twice 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> * Note which library version TestHostBigQueryRpc is tied to and when to 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> * Build the mock server's origin in one place, testHostOf() 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> * Match the harness self-test on runWithMockServer()'s own message 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> * Put the mock-server test comments where the formatter leaves them alone 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> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
… 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>
* Let the request cursor, not a count assertion, report 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 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> * Check that every request was walked when the cursor is closed 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> * Return both upload requests from assertLoadAttempt() 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> * Say which request the test asserts when the run recorded fewer 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> --------- 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
Integration branch collecting the changes that bring this plugin's behavior in line with the ruby plugin
(embulk-output-bigquery), ported one concern per PR. Each PR is reviewed and merged into
feature/ruby-compatibility;this PR merges the whole set into
masteronce everything is in.Some of the changes are breaking (marked below): existing configs may start failing, or produce different output
values. Users need to be notified before this is released.
PRs
Test infrastructure:
Stacked in order (each is based on the previous one, so merge top to bottom):
Independent:
PR-10 Add source_format/compression validation and fix project_id extraction bugFound while reviewing the PRs above (gaps the java port has had from the start):
load()returns null for a missing file and the transaction report NPEs, ruby reports 0 rows as a row-count mismatch)updateTableIfNeed(), which changes whetherreplaceempties the destination)Test plan
./gradlew teston this branch after every merge./gradlew spotlessApplyleaves no diff