test: run bun handwritten tests in bun runtime for firestore through pubsub plus spanner-driver

[danieljbruce]

Description

Updates handwritten packages across the repository to use the runtime-agnostic test runner (bin/run-test.cjs) and addresses test compatibility issues when running under the Bun runtime:

• Migrate test scripts to run-test.cjs: Updated test and system-test scripts in package.json for bigquery, bigquery-storage, datastore, error-reporting, firestore, google-cloud-dns, logging, logging-bunyan, logging-winston, pubsub, and spanner-driver to use node ../../bin/run-test.cjs --config ../../.mocharc.cjs.
• handwritten/datastore:
  • Added @grpc/grpc-js and @grpc/proto-loader to devDependencies and updated pnpm-lock.yaml to ensure mock server tests resolve gRPC dependencies under pnpm's strict package isolation.
  • Accommodated JavaScriptCore / Bun null property error message formatting ("null is not an object (evaluating 'data.name.toString')") alongside existing V8/Node patterns in test/index.ts.
• handwritten/bigquery-storage:
  • Adjusted proto test data types and timestamp formatting in test/adapt/proto.ts so deepStrictEqual assertions and Prettier formatting pass cleanly across runtimes.
• handwritten/error-reporting:
  • Sanitized environment variables in unit tests (configuration.ts, service-configuration.ts) to avoid cross-test pollution during non-parallel execution.
• handwritten/logging:
  • Explicitly initialized instrumentation.setInstrumentationStatus(true) in test/log-sync.ts beforeEach to prevent automatic diagnostic log emission from polluting file stream output during sequential test runs.
• handwritten/pubsub:
  • Fixed a test deadlock in test/message-queues.ts (ModAckQueue -> should send call options) by capturing the completion promise and flushing the queue before awaiting completion.

Impact

• Enables unit and system tests for these handwritten packages to execute successfully under both Node.js and the Bun runtime (bun --bun run test).
• Resolves flaky timeouts, environmental variable leakage, and runtime-specific assertion discrepancies across the affected test suites without altering public APIs or production behavior.
• Ensures all workspace package dependencies are properly declared and satisfied under strict pnpm package isolation in CI.

Scope

Libraries included: firestore, google-cloud-dns, logging, logging-bunyan, logging-winston, pubsub
Remaining: bigtable, spanner, storage

We also updated the handwritten libraries that are already done so that they use the test-runner with a simpler script without updating gapic tools.

[danieljbruce]

/gemini review

[gemini-code-assist]

Code Review

This pull request updates the test execution scripts across multiple packages to use a centralized test runner (run-test.cjs) with a shared Mocha configuration. Additionally, it resolves test-specific issues, including disabling diagnostic instrumentation during LogSync tests to prevent unexpected log entries, and refactoring a pubsub test to avoid a deadlock under Bun by storing the add() promise and awaiting it after calling flush(). The feedback suggests using Promise.all to await both the add completion and flush concurrently, which prevents potential unhandled promise rejections if flush fails.

[gemini-code-assist]

Awaiting modAckQueue.flush('test') before completion can lead to an unhandled promise rejection if flush throws an error. In Node.js and Bun, unhandled promise rejections can cause the test runner process to exit abruptly or fail. Using Promise.all to await both promises concurrently ensures that any rejection from either promise is properly handled.

Suggested change

	const completion = modAckQueue.add(new FakeMessage() as Message, 10);
	await modAckQueue.flush('test');
	await completion;
	const completion = modAckQueue.add(new FakeMessage() as Message, 10);
	await Promise.all([completion, modAckQueue.flush('test')]);

[danieljbruce]

This is a good one. We don't want hidden failures.

[danieljbruce]

/gemini review

[gemini-code-assist]

Code Review

This pull request updates test execution scripts across multiple packages to use a centralized test runner script (run-test.cjs) with a shared Mocha configuration. Additionally, it addresses test-specific issues, such as disabling diagnostic instrumentation during LogSync tests and resolving a potential deadlock under Bun in MessageQueues tests. Feedback on the changes suggests wrapping the file cleanup in LogSync's afterEach hook in a try...finally block to ensure the global instrumentation status is always reset, even if file deletion fails.

[gemini-code-assist]

If fs.rmSync throws an error (for example, due to file locking issues on certain platforms like Windows), the subsequent call to reset the global instrumentation status will be skipped. Wrapping the file cleanup in a try...finally block ensures that the instrumentation status is always reset, preventing side effects and potential flakiness in other tests.

    afterEach(() => {
      try {
        fs.rmSync(TEST_FILE, {force: true});
      } finally {
        instrumentation.setInstrumentationStatus(false);
      }
    });

[danieljbruce]

resolved

[danieljbruce]

/gemini review

[gemini-code-assist]

Code Review

This pull request standardizes test execution across multiple packages by replacing direct mocha and c8 invocations with a centralized test runner script ('run-test.cjs'). It also addresses a deadlock issue in the Pub/Sub tests under Bun by separating the queue addition and flushing operations, and disables automatic diagnostic instrumentation during logging tests. The reviewer suggested simplifying the Pub/Sub test fix by sequentially awaiting the flush and completion promises instead of using Promise.all.

[gemini-code-assist]

Instead of using Promise.all to concurrently await the completion promise and the flush operation, you can sequentially await them. Since add() synchronously queues the message and returns a promise, calling flush() immediately after will trigger the flush and resolve the completion promise. Awaiting them sequentially is simpler, avoids the complexity of Promise.all, and prevents potential unhandled promise rejections if one of them fails.

Suggested change

	const completion = modAckQueue.add(new FakeMessage() as Message, 10);
	await Promise.all([completion, modAckQueue.flush('test')]);
	const completion = modAckQueue.add(new FakeMessage() as Message, 10);
	await modAckQueue.flush('test');
	await completion;

[danieljbruce]

This is the opposite of what was suggested 2 suggestions ago. I think we can stick with the code as it is.

[bshaffer]

Can we simplify node ../../bin/run-test.cjs --config ../../.mocharc.cjs build/test to just run-test build/test?

1. If we expose run-test in bin on a workspace package (e.g. gapic-tools or a package in core/dev-packages) and add it to the root package.json devDependencies, pnpm will link it into the root node_modules/.bin so it is on PATH for all workspace packages.
2. We can drop --config ../../.mocharc.cjs — Mocha already walks up directories to find the root .mocharc.cjs when no local config exists, and passing --config ../../.mocharc.cjs here overrides the package-local .mocharc.js configs in handwritten/* (like firestore/.mocharc.js).

[danieljbruce]

Can we simplify node ../../bin/run-test.cjs --config ../../.mocharc.cjs build/test to just run-test build/test?

1. If we expose run-test in bin on a workspace package (e.g. gapic-tools or a package in core/dev-packages) and add it to the root package.json devDependencies, pnpm will link it into the root node_modules/.bin so it is on PATH for all workspace packages.
2. We can drop --config ../../.mocharc.cjs — Mocha already walks up directories to find the root .mocharc.cjs when no local config exists, and passing --config ../../.mocharc.cjs here overrides the package-local .mocharc.js configs in handwritten/* (like firestore/.mocharc.js).

Yeah. I looked into this and the windows tests seem to have trouble with .. which can be addressed by prepending with node, but I did what you said and added a link to tools for all workspace packages.

[danieljbruce]

Can we simplify node ../../bin/run-test.cjs --config ../../.mocharc.cjs build/test to just run-test build/test?

1. If we expose run-test in bin on a workspace package (e.g. gapic-tools or a package in core/dev-packages) and add it to the root package.json devDependencies, pnpm will link it into the root node_modules/.bin so it is on PATH for all workspace packages.
2. We can drop --config ../../.mocharc.cjs — Mocha already walks up directories to find the root .mocharc.cjs when no local config exists, and passing --config ../../.mocharc.cjs here overrides the package-local .mocharc.js configs in handwritten/* (like firestore/.mocharc.js).

Yeah. I looked into this and the windows tests seem to have trouble with .. which can be addressed by prepending with node, but I did what you said and added a link to tools for all workspace packages.

Actually, I suggest we do #2, but not #1.

Adding a link to the gapic-tools directory doesn't work because it makes the system tests fail. GCB does not include the gapic-tools directory unless you add it to devDependencies which isn't worth it for a minor simplification.