Repository navigation
test: remove --proxyquire-shim from bigtable, spanner, and storage - #9641
danieljbruce wants to merge 4 commits into
Conversation
![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 removes the --proxyquire-shim flag from test scripts across multiple packages, introducing a local proxyquire utility and updating test imports, while replacing it with sinon stubs or custom module loader overrides in other areas. The feedback suggests refactoring several tests in bigtable, pubsub, and spanner to use async/await and assert.rejects instead of silencing linter rules with eslint-disable-next-line for unhandled promises or missing returns, which will prevent potential silent test failures and improve code quality.
| // eslint-disable-next-line promise/catch-or-return, promise/always-return | ||
| bigtable.close().then(() => { |
There was a problem hiding this comment.
Silencing the linter with eslint-disable-next-line for unhandled promises can lead to silent test failures or hangs if bigtable.close() rejects, as the done callback would never be called. Instead of disabling the linter, consider refactoring the test to use async/await and removing the done callback entirely.
| // eslint-disable-next-line promise/always-return | ||
| void queue.publishDrain().then(() => { |
There was a problem hiding this comment.
Instead of disabling the promise/always-return linter rule, consider refactoring the test to use async/await. This avoids callback nesting, simplifies the asynchronous flow, and eliminates the need for linter overrides.
| // eslint-disable-next-line promise/always-return | ||
| void queue.publish('test').then(() => { |
There was a problem hiding this comment.
Instead of disabling the promise/always-return linter rule, consider refactoring the test to use async/await to simplify the asynchronous flow and avoid linter overrides.
| // eslint-disable-next-line promise/always-return | ||
| .then(() => { |
There was a problem hiding this comment.
Instead of disabling the promise/always-return linter rule, consider refactoring the test to use async/await with assert.rejects. This is much cleaner, avoids callback-based assertions, and eliminates the need for linter overrides.
1 participant
Description
Replaces global proxyquire shim with local test helper in three packages.
Impact
Removes --proxyquire-shim from bigtable, spanner, and storage unit test scripts.
Changes
handwritten/bigtable,handwritten/spanner,handwritten/storage: Add localtest/proxyquire.tshelper built onModule.prototype.requireandrequire.cacheand remove--proxyquire-shimfrom theirpackage.jsontest scripts.handwritten/bigtable/test/base64keymap.ts: Remove unusedproxyquireimport.handwritten/spanner/observability-test/helper.ts&spanner.ts: Callcontext.disable()insetGlobalContextManagerand re-register the active context manager inEndToEndbeforeEachso OpenTelemetry context propagation remains isolated across test suites.Testing
pnpm testunder both Node.js and Bun (JS_RUNTIME=bun) inhandwritten/bigtable,handwritten/spanner, andhandwritten/storage.bin/linter.mjs --strict.Alternatives
bin/proxyquire-bun-shim.cjspreload shim.