Skip to content

test(storage): adopt run-test runner and fix Bun shim caching and mocking for storage - #9458

Merged
danieljbruce merged 66 commits into
mainfrom
bun-runtime/1-test-runner-handwritten-libraries-4
Sep 30, 2026
Merged

danieljbruce merged 66 commits into
mainfrom
bun-runtime/1-test-runner-handwritten-libraries-4

Conversation

@danieljbruce

@danieljbruce danieljbruce commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Description

  • Updated test, system-test, and conformance test scripts in handwritten/storage/package.json to invoke bin/run-test.cjs.
  • Added Module._load delegation to Module.prototype.require in bin/proxyquire-bun-shim.cjs so tests that temporarily monkeypatch Module._load for dynamic import recovery execute properly in Bun.
  • Implemented generational cache snapshotting for Module._cache and require.cache in bin/proxyquire-bun-shim.cjs so module mocking tools like mockery and proxyquire can isolate clean caches and restore previously loaded modules without cross-test pollution.
  • Replaced native fetch usage in globalThis.__googleCloudBunFetch with Node's http.request and https.request implementations in bin/proxyquire-bun-shim.cjs so network requests under Bun are transparently intercepted by nock.

Impact

This change allows the test suites for Storage to execute seamlessly under both Node.js and Bun runtimes without modifying library implementation code. It resolves critical Bun test failures where HTTP requests bypassed nock interception and module cache wipes corrupted cached class definitions across test files. As a result, all 1,278 unit tests in Storage now pass cleanly under the Bun runtime while fully preserving standard test execution and coverage under Node.js.

Testing

Ran bun --bun run test (as well as JS_RUNTIME=bun node ../../bin/run-test.cjs build/cjs/test) in handwritten/storage to verify that all 1,278 unit tests pass with zero failures under Bun. In addition, ran node ../../bin/run-test.cjs --no-c8 build/cjs/test to ensure backward compatibility and verify all 1,278 unit tests continue passing under Node.js.

Note that now we only use the fetch shim for unit tests because unit tests use nock which bun avoids without the shim, but with the system tests we do not want to use the shim to steer the tests toward the mock because we are evaluating the behaviour of the whole client.

quirogas and others added 30 commits September 21, 2026 21:26
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.
…ogging-bunyan, logging-winston, pubsub, spanner, spanner-driver, and storage
…into bun-runtime/1-test-runner-handwritten-libraries

# Conflicts:
#	core/packages/gax/.mocharc.js
…into bun-runtime/1-test-runner-handwritten-libraries-2
…gging, logging-bunyan, logging-winston, pubsub, spanner, spanner-driver, and storage"

This reverts commit 8088cf4.
@danieljbruce
danieljbruce requested review from a team as code owners September 28, 2026 15:51
@product-auto-label product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Sep 28, 2026
@github-actions
github-actions Bot requested a review from shivanee-p September 28, 2026 15:51

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a Bun proxyquire shim (bin/proxyquire-bun-shim.cjs) that overrides module caching and implements a custom __googleCloudBunFetch utility using Node's http/https modules to ensure compatibility under Bun. It also standardizes test execution across multiple packages by routing them through run-test.cjs, and resolves specific test issues in logging and pubsub to prevent deadlocks and unexpected logs. The review feedback highlights three key issues in the proxyquire shim: first, checking init.headers.entries is insufficient because arrays also have an entries method, which could corrupt headers; second, __googleCloudBunFetch should explicitly support Uint8Array and ArrayBuffer request bodies to avoid sending empty payloads; and third, Module._load should be called statically rather than with an instance context (this) to prevent errors in monkeypatched environments.

Comment on lines +350 to +359
if (
init.headers instanceof Headers ||
(init.headers && typeof init.headers.entries === 'function')
) {
for (const [k, v] of init.headers.entries()) {
headers[k] = v;
}
} else {
headers = {...init.headers};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Checking typeof init.headers.entries === 'function' is not sufficient to distinguish a Headers object from a plain Array, because Array.prototype.entries also exists but returns an iterator of [index, element] instead of [key, value]. If init.headers is passed as an array of key-value pairs, this will corrupt the headers object. We should explicitly handle the Array case first.

        if (init.headers instanceof Headers) {
          for (const [k, v] of init.headers.entries()) {
            headers[k] = v;
          }
        } else if (Array.isArray(init.headers)) {
          for (const [k, v] of init.headers) {
            headers[k] = v;
          }
        } else if (typeof init.headers.entries === 'function') {
          for (const [k, v] of init.headers.entries()) {
            headers[k] = v;
          }
        } else {
          headers = {...init.headers};
        }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed

Comment on lines +460 to +465
} else if (
typeof init.body === 'string' ||
Buffer.isBuffer(init.body)
) {
req.write(init.body);
req.end();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current implementation only writes the request body if it is a string or a Node Buffer. However, in modern JS runtimes (and especially under Bun), request bodies are frequently passed as Uint8Array or ArrayBuffer (e.g., when sending serialized protobufs). If a Uint8Array is passed, it will fall through to the else block and send an empty body. We should explicitly support Uint8Array and ArrayBuffer.

            } else if (
              typeof init.body === 'string' ||
              Buffer.isBuffer(init.body) ||
              init.body instanceof Uint8Array ||
              init.body instanceof ArrayBuffer
            ) {
              const chunk = init.body instanceof ArrayBuffer ? new Uint8Array(init.body) : init.body;
              req.write(chunk);
              req.end();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed

Comment on lines +669 to +671
return patchGaxiosIfPresent(
Module._load.call(this, id, this, /* isMain */ false),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Module._load is a static method on the Module class, not an instance method. Calling it with this (which is a Module instance inside Module.prototype.require) as the this context can cause unexpected behavior or errors in monkeypatched implementations of _load that expect this to be the Module class. It should be called directly as Module._load.

      return patchGaxiosIfPresent(
        Module._load(id, this, /* isMain */ false),
      );

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed

@danieljbruce danieljbruce changed the title test(storage): adopt run-test runner and fix Bun shim caching and mocking test(storage): adopt run-test runner and fix Bun shim caching and mocking for storage Sep 28, 2026
@danieljbruce
danieljbruce changed the base branch from main to bun-runtime/1-test-runner-handwritten-libraries-3 September 28, 2026 17:08
Base automatically changed from bun-runtime/1-test-runner-handwritten-libraries-3 to main September 28, 2026 18:57
@quirogas
quirogas removed request for a team September 29, 2026 14:25
@danieljbruce
danieljbruce force-pushed the bun-runtime/1-test-runner-handwritten-libraries-4 branch from 18f3f92 to 5101c0b Compare September 29, 2026 21:17
@danieljbruce
danieljbruce requested a review from a team as a code owner September 29, 2026 21:17
@danieljbruce
danieljbruce force-pushed the bun-runtime/1-test-runner-handwritten-libraries-4 branch from 5101c0b to 1a5185d Compare September 29, 2026 21:24
@danieljbruce
danieljbruce merged commit f11413c into main Sep 30, 2026
51 checks passed
@danieljbruce
danieljbruce deleted the bun-runtime/1-test-runner-handwritten-libraries-4 branch September 30, 2026 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants