test(spanner): handle Node 18 prototype equality in PartialResultStream row assertions - #9294
Conversation
…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.
There was a problem hiding this comment.
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.
| // 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); | ||
| } |
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this comment.
We want to get rid of this, when we move to Node 22.
Node 18's
assert.deepStrictEqualstrictly enforces prototype equality (Object.getPrototypeOf(a) === Object.getPrototypeOf(b)), which causes assertions comparingRowImpl(an Array subclass withconstructor: 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
PartialResultStreamrow emission unit tests to spreadrowinto a standard Array on Node < 20 while assertingrowdirectly on Node 20+, keeping tests green across all supported Node versions without altering theRowImplperformance 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:
Fixes #<issue_number_goes_here> 🦕