test(test-utils): Add MemoryProfiler for heap snapshot testing via CDP#20555
test(test-utils): Add MemoryProfiler for heap snapshot testing via CDP#20555
Conversation
size-limit report 📦
|
JPeer264
force-pushed
the
jp/e2e-test-memory-profiler
branch
8 times, most recently
from
1a0c0e3 to
55510e9
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| this._connected = false; | ||
| }); | ||
|
|
||
| this._ws.on('message', (data: Buffer) => { |
There was a problem hiding this comment.
This function is a bit high in complexity. Maybe you can refactor that a bit.
| private readonly _gcSettleDelayMs: number; | ||
| private _initialized: boolean; | ||
|
|
||
| readonly #debug: boolean; |
There was a problem hiding this comment.
The # would be syntax with an actual meaning in JS: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_elements
I would rather remove it from the variable name. But you could use it for the private fields above to have runtime-safe private fields. So instead of private _variable, you would have just #variable.
| readonly #debug: boolean; | |
| readonly debug: boolean; |
There was a problem hiding this comment.
Exactly that was the goal here, it is here on purpose. I'll add it to the others
|
|
||
| private async _collectGarbage(): Promise<void> { | ||
| // Multiple GC passes to ensure full collection - some V8 inspectors need this | ||
| for (let i = 0; i < 3; i++) { |
There was a problem hiding this comment.
There are different steps in GCing. Usually one is enough, but I wanted to make sure that everything is really garbage collected and therefore only leaks are getting tested:
For that purpose, multiple rounds of work and additional tweaks to the garbage collection process may be needed since some steps (e.g. invocation of weak callbacks) are not designed to be run immediately.
(https://joyeecheung.github.io/blog/2023/12/30/fixing-nodejs-vm-apis-3/)
| let totalSize = 0; | ||
|
|
||
| if (selfSizeIdx !== -1) { | ||
| for (let i = 0; i < snapshot.nodes.length; i += nodeFieldCount) { |
There was a problem hiding this comment.
You could use a for of here as you are iterating over the nodes
There was a problem hiding this comment.
Don't think that would work as I don't step directly over snapshot.nodes and don't increment i by one with i++ but with i += nodeFieldCount
JPeer264
force-pushed
the
jp/e2e-test-memory-profiler
branch
from
ba673f9 to
dc1877f
Compare
| nodeGrowthPercent: (nodeGrowth / baseline.nodeCount) * 100, | ||
| edgeGrowth, | ||
| edgeGrowthPercent: (edgeGrowth / baseline.edgeCount) * 100, | ||
| sizeGrowth, | ||
| sizeGrowthPercent: (sizeGrowth / baseline.totalSize) * 100, |
There was a problem hiding this comment.
Bug: The compareSnapshots function may divide by zero if a baseline snapshot has a totalSize, nodeCount, or edgeCount of 0, resulting in NaN values.
Severity: MEDIUM
Suggested Fix
In compareSnapshots, before calculating growth percentages, check if the denominators (baseline.nodeCount, baseline.edgeCount, baseline.totalSize) are zero. If a denominator is zero, the corresponding growth percentage should be set to 0 to avoid division by zero and prevent NaN values.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: dev-packages/test-utils/src/memory-profiler.ts#L241-L245
Potential issue: The `compareSnapshots` function calculates growth percentages by
dividing by metrics from a baseline snapshot, such as `baseline.totalSize`. The logic in
`#parseSnapshotStats` can result in `totalSize` being 0 if the heap snapshot does not
contain a `self_size` field, which is possible across different V8 environments. This
leads to a division-by-zero when calculating `sizeGrowthPercent`, silently producing
`NaN` in the returned result object. The same risk applies to `nodeCount` and
`edgeCount`.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dc1877f. Configure here.
| export interface HeapUsage { | ||
| usedSize: number; | ||
| totalSize: number; | ||
| } |
There was a problem hiding this comment.
HeapUsage interface exported but never used anywhere
Low Severity
The HeapUsage interface is defined in cdp-client.ts and re-exported from index.ts, but it is never imported or referenced by any code in the codebase. It appears to be dead code — possibly a leftover from an earlier design that used Runtime.getHeapUsage before switching to the snapshot-based approach.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit dc1877f. Configure here.
2 participants
Adds CDPClient and MemoryProfiler to test-utils for V8 heap profiling. This PR prevents #20407 entirely by comparing heap snapshots.
Within a Playwright test following can now be used:
This works by using the Chrome Developer Protocol (CDP). There is also a CDPSession API available from Playwright, but that would only work for sessions which run in the browser. Theoretically, this could also work in integration tests, but the idea is that this could in the future also be extended to use the CDPSession from Playwright for browser tests.