test(spanner): adopt runtime-agnostic test runner and Bun compatibility - #9460
Conversation
Adds bin/run-test.cjs and bin/proxyquire-bun-shim.cjs to run Mocha tests across both Node.js and Bun without breaking Node coverage or parallelism. When invoked under Node.js, bin/run-test.cjs delegates to c8 and Mocha with worker-thread parallelism enabled. When invoked under Bun (via bun --bun or JS_RUNTIME=bun), it skips c8, disables Mocha worker threads (--no-parallel), preloads the Bun proxyquire compatibility shim, and executes Mocha directly in-process so #!/usr/bin/env node shebangs do not silently switch execution back to Node.js.
…gnal timeout override
…ogging-bunyan, logging-winston, pubsub, spanner, spanner-driver, and storage
…into bun-runtime/1-test-runner-handwritten-libraries # Conflicts: # core/packages/gax/.mocharc.js
This reverts commit 5a29104.
This reverts commit c9446ab.
…ps://github.com/googleapis/google-cloud-node into bun-runtime/1-test-runner-handwritten-libraries
This reverts commit 4c4a74d.
…ime" This reverts commit 486a301.
…bun runtime"" This reverts commit 687e386.
This reverts commit 756cc45.
…into bun-runtime/1-test-runner-handwritten-libraries-2
…ttps://github.com/googleapis/google-cloud-node into bun-runtime/1-test-runner-handwritten-libraries-2
…th bun tests instead
…gging, logging-bunyan, logging-winston, pubsub, spanner, spanner-driver, and storage" This reverts commit 8088cf4.
danieljbruce
changed the title
…runner-handwritten-libraries-4-1 # Conflicts: # bin/proxyquire-bun-shim.cjs
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several compatibility fixes and enhancements for running tests under the Bun runtime, particularly focusing on Spanner and Storage. Key changes include adding a design document for an opt-in Bun fetch transport shim, updating proxyquire and assert shims in bin/proxyquire-bun-shim.cjs to handle loose deep equality and prototype issues in Bun, and adapting Spanner observability tests to correctly manage OpenTelemetry context managers and sort spans/events using high-resolution time arrays. Feedback is provided on optimizing array cloning in the assert.deepStrictEqual shim to preserve sparse array semantics and improve performance.
… assert.deepStrictEqual shim
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several updates to the Spanner client tests to improve compatibility with Bun and Node.js environments, including updating error message assertions, fixing span and event sorting logic, and wrapping test executions with a custom runner. Additionally, it integrates OpenTelemetry tracing setup across multiple test suites. The review feedback highlights two key improvement opportunities: correcting a type mismatch and preventing resource leaks in spanner.ts by instantiating the tracer provider in a before block, and addressing a Mocha anti-pattern in session-pool.ts by moving tracer provider instantiation from the describe block to a before hook with proper cleanup.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces test compatibility improvements for Bun and Node.js, including wrapping test executions, updating error assertions, and fixing incorrect span sorting logic. It also integrates OpenTelemetry context managers into several test suites. Feedback highlights a critical issue in batch-transaction.ts where the global context manager is registered in beforeEach instead of once at the suite level, which can cause subsequent tests to run with a disabled context manager.
…saction observability test
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several updates to improve compatibility with the Bun runtime, including handling Bun-specific error messages, prototype equality checks, and readonly property assignment errors. It also fixes several bugs in the test suite where span and event sorting logic was broken due to incorrect comparison operators, updating them to correctly compare high-resolution time arrays. Additionally, test execution scripts in package.json have been updated to use a custom test runner shim, and OpenTelemetry context managers and tracer providers are now properly registered and cleaned up across tests. No review comments were provided, so there is no feedback to address.
feywind
left a comment
feywind
left a comment
There was a problem hiding this comment.
I think the most interesting thing here is the possibility of building up some runtime-agnostic utilities for testing and such. It would help if we want Deno later too.
| const {SimpleSpanProcessor} = require('@opentelemetry/sdk-trace-base'); | ||
| import {Session, Spanner} from '../src'; | ||
| import * as bt from '../src/batch-transaction'; | ||
| const { |
There was a problem hiding this comment.
I'm sure there's a reason for it given that there's another above, but I'm curious why the mix of require and import?
There was a problem hiding this comment.
This follows the existing pattern across the observability-test suite (e.g., observability-test/spanner.ts), where the OpenTelemetry packages and ./helper were originally loaded via require() (in part because NodeTracerProvider is passed an untyped exporter property in its config object, which would fail strict TypeScript checks if imported via ES import, and @opentelemetry/sdk-trace-base was originally a transitive dependency). We kept require() here to stay consistent with the rest of the file.
There was a problem hiding this comment.
I also noticed when we did the Node upgrade last year that we had to play with require/import a lot to eliminate compiler errors so that might be another reason we see this discrepancy.
| assert.throws(() => { | ||
| replaceProjectIdToken(frozenObj, projectId); | ||
| }, /Cannot assign to read only property/); | ||
| }, /Cannot assign to read only property|Attempted to assign to readonly property/); |
There was a problem hiding this comment.
Not really commentary for this PR, but it might be cool to collect some of these differences in test-utils if we are supporting bun going forward anyway. Or maybe find other way to do these checks that don't rely on strings. (Again, different task...)
There was a problem hiding this comment.
Agreed — centralizing runtime-agnostic error assertions and environment checks in test-utils (or asserting on the error type rather than engine-specific message strings) will be much cleaner as we expand Bun (and potentially Deno) support. We'll track this as a follow-up task.
| // Node 20+ relaxed this for Array subclasses with constructor = Array. | ||
| if (parseInt(process.versions.node.split('.')[0], 10) < 20) { | ||
| if ( | ||
| parseInt(process.versions.node.split('.')[0], 10) < 20 || |
There was a problem hiding this comment.
Same for this kind of thing. We could have predicates in test-utils that do this sort of test.
There was a problem hiding this comment.
Agreed — adding runtime capability predicates or assertion helpers in test-utils so individual package tests don't need inline process.versions checks makes sense as a follow-up.
Use assertArraySubclassStrictEqual, isReadOnlyPropertyError, and isNullOrUndefinedPropertyError from google-test-utils in Spanner unit tests to address PR #9460 review feedback. [Generated-by: AI]
…ssertion helpers (#9492) ### Summary Follows up on review feedback from @feywind on #9460 by adding the shared runtime detection predicates and cross-engine assertion helpers to `google-test-utils` (`core/test-utils`) and adopting them in the `@google-cloud/spanner` (`handwritten/spanner`) unit tests she commented on: - **Runtime environment predicates** (`isNode`, `isBun`, `getNodeMajorVersion`, `RuntimeVersions`): Centralizes detection of Node.js vs. Bun so tests do not need ad-hoc `process.versions` parsing ([discussion_r4149070743](https://togithub.com/googleapis/google-cloud-node/pull/9460#discussion_r4149070743)). - **Array subclass deep-strict equality helpers** (`requiresStrictArrayPrototypeEquality`, `assertArraySubclassStrictEqual`): Centralizes the workaround for Node.js < 20 and Bun where `assert.deepStrictEqual` strictly enforces prototype equality when comparing an `Array` subclass (such as Spanner's `RowImpl`) against a plain `Array` literal ([discussion_r4149239787](https://togithub.com/googleapis/google-cloud-node/pull/9460#discussion_r4149239787)). - **Cross-engine read-only property `TypeError` matcher** (`READONLY_PROPERTY_ERROR_REGEX`, `isReadOnlyPropertyError`): Centralizes matching for read-only property assignment errors across V8 (Node.js) and JavaScriptCore (Bun) ([discussion_r4149233766](https://togithub.com/googleapis/google-cloud-node/pull/9460#discussion_r4149233766)). - **Spanner test adoption** (`handwritten/spanner`): - `handwritten/spanner/test/helper.ts`: Replaces the inline V8/JSC read-only property regex with `isReadOnlyPropertyError(err, 'name')`. - `handwritten/spanner/test/partial-result-stream.ts`: Replaces the two inline `process.versions` checks with `assertArraySubclassStrictEqual(row, EXPECTED_ROW)`. ### Testing - Added unit tests in `core/test-utils/test/runtime.test.ts` covering all exported functions (100% statement/function/line coverage). - Verified `pnpm test` and `pnpm run lint` in `core/test-utils`. - Verified updated unit tests (`helper`, `partial-result-stream`) and ESLint/TypeScript checks in `handwritten/spanner`.
3 participants
Description
This PR migrates
@google-cloud/spannerto the runtime-agnostic test runner (bin/run-test.cjs) and resolves test compatibility issues under the Bun runtime.Impact
@google-cloud/spannerunit, system, and observability test suites to execute seamlessly under Bun while maintaining full fidelity and test coverage across Node.js (v22, v24, v26).src/is completely untouched.afterEach, avoiding test pollution and flaky observability assertions.Key Changes
Adopt
run-test.cjsin@google-cloud/spanner:test,system-test, andobservability-testscripts inhandwritten/spanner/package.jsonto execute throughbin/run-test.cjs.Observability Tests & OpenTelemetry Lifecycle:
contextManagerusingAsyncLocalStorageContextManager(with fallback toAsyncHooksContextManager) for proper trace context propagation under Bun.disableContextAndManager(contextManager)cleanup calls inafterEachacross observability test suites (batch-transaction.ts,spanner.ts) to prevent context leakage across test runs.batch-transaction.ts,table.ts, andtransaction.tsto return numeric differences (spanA.duration[0] - spanB.duration[0]orspanA.startTime[0] - spanB.startTime[0]) rather than boolean comparisons.Assertion & Compatibility Shim Enhancements (
bin/proxyquire-bun-shim.cjs):assert.deepEqualwith a safelooseDeepEqualfallback for primitive values,Date,RegExp,Buffer,Map, andSetto mirror Node.js loose equality semantics under Bun.assert.deepStrictEqualto address Bun <= 1.4 Array subclass constructor matching while preserving own properties.Array.prototype.sortoverride in favor of clean numeric comparators in test code.Session Pool Test Stability:
session-pool.tsto ensure consistent session acquisition and eviction assertions across runtimes.Testing
presubmit-bun/units).spanner-system-tests&spanner-regular-sessions-system-tests) passing in Cloud Build.