Skip to content

test(spanner): handle Node 18 prototype equality in PartialResultStream row assertions - #9294

Merged
sakthivelmanii merged 1 commit into
mainfrom
fix-spanner-node18-deep-strict-equal
Sep 11, 2026
Merged

test(spanner): handle Node 18 prototype equality in PartialResultStream row assertions#9294
sakthivelmanii merged 1 commit into
mainfrom
fix-spanner-node18-deep-strict-equal

Conversation

@sakthivelmanii

Copy link
Copy Markdown
Contributor

Node 18's assert.deepStrictEqual strictly enforces prototype equality (Object.getPrototypeOf(a) === Object.getPrototypeOf(b)), which causes assertions comparing RowImpl (an Array subclass with constructor: Array) against plain Array literals (EXPECTED_ROW) to fail with "Values have same structure but are not reference-equal".

Add a Node major version check in PartialResultStream row emission unit tests to spread row into a standard Array on Node < 20 while asserting row directly on Node 20+, keeping tests green across all supported Node versions without altering the RowImpl performance optimizations.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

…am row assertions

Node 18's `assert.deepStrictEqual` strictly enforces prototype equality
(`Object.getPrototypeOf(a) === Object.getPrototypeOf(b)`), which causes
assertions comparing `RowImpl` (an Array subclass with `constructor: Array`)
against plain Array literals (`EXPECTED_ROW`) to fail with
"Values have same structure but are not reference-equal".

Add a Node major version check in `PartialResultStream` row emission unit
tests to spread `row` into a standard Array on Node < 20 while asserting
`row` directly on Node 20+, keeping tests green across all supported Node
versions without altering the `RowImpl` performance optimizations.
@sakthivelmanii
sakthivelmanii requested a review from a team as a code owner September 11, 2026 17:14
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Sep 11, 2026
@github-actions
github-actions Bot requested a review from shivanee-p September 11, 2026 17:14

@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 updates tests in handwritten/spanner/test/partial-result-stream.ts to handle Node 18's strict prototype equality checks in assert.deepStrictEqual by conditionally spreading row based on the Node version. The reviewer suggests simplifying this logic by always spreading row into a plain array, which works consistently across all Node versions and eliminates the need for version-specific branching.

Comment thread handwritten/spanner/test/partial-result-stream.ts
Comment on lines +260 to +267
// Node 18's assert.deepStrictEqual strictly requires prototype equality,
// which fails when comparing RowImpl (an Array subclass) with a plain Array literal.
// Node 20+ relaxed this for Array subclasses with constructor = Array.
if (parseInt(process.versions.node.split('.')[0], 10) < 20) {
assert.deepStrictEqual([...row], EXPECTED_ROW);
} else {
assert.deepStrictEqual(row, EXPECTED_ROW);
}

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

Instead of parsing process.versions.node and branching based on the Node version, we can simplify the test by always spreading row into a plain array ([...row]). This works consistently across all Node versions, avoids version-specific logic, and makes the test code much cleaner and easier to maintain.

Suggested change
// Node 18's assert.deepStrictEqual strictly requires prototype equality,
// which fails when comparing RowImpl (an Array subclass) with a plain Array literal.
// Node 20+ relaxed this for Array subclasses with constructor = Array.
if (parseInt(process.versions.node.split('.')[0], 10) < 20) {
assert.deepStrictEqual([...row], EXPECTED_ROW);
} else {
assert.deepStrictEqual(row, EXPECTED_ROW);
}
// Spread row into a plain array to avoid prototype equality failures in Node 18's assert.deepStrictEqual.
assert.deepStrictEqual([...row], EXPECTED_ROW);

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.

We want to get rid of this, when we move to Node 22.

@sakthivelmanii
sakthivelmanii merged commit 82948b8 into main Sep 11, 2026
51 checks passed
@sakthivelmanii
sakthivelmanii deleted the fix-spanner-node18-deep-strict-equal branch September 11, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants