Skip to content

Commit 3457aee

Browse files
joyeecheungRafaelGSS
joyeecheung
authored and
RafaelGSS
committed
test: use validateByRetainingPath in heapdump tests
This makes sure that the tests are run on actual heap snapshots and prints out missing paths when it cannot be found, which makes failures easier to debug, and removes the unnecessary requirement for BaseObjects to be root - which would make the heap snapshot containment view rather noisy and is not conceptually correct, since they are actually held by the BaseObjectList held by the realms. PR-URL: #57417 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
1 parent bf9f257 commit 3457aee

12 files changed

+238
-204
lines changed

‎test/common/heap.js

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@ function validateSnapshotNodes(...args) {
221221
* A alternative heap snapshot validator that can be used to verify cppgc-managed nodes.
222222
* Modified from
223223
* https://chromium.googlesource.com/v8/v8/+/b00e995fb212737802810384ba2b868d0d92f7e5/test/unittests/heap/cppgc-js/unified-heap-snapshot-unittest.cc#134
224+
* @param {object[]} nodes Snapshot nodes returned by createJSHeapSnapshot() or a subset filtered from it.
224225
* @param {string} rootName Name of the root node. Typically a class name used to filter all native nodes with
225226
* this name. For cppgc-managed objects, this is typically the name configured by
226227
* SET_CPPGC_NAME() prefixed with an additional "Node /" prefix e.g.
@@ -231,12 +232,12 @@ function validateSnapshotNodes(...args) {
231232
* node_type?: string,
232233
* edge_type?: string,
233234
* }]} retainingPath The retaining path specification to search from the root nodes.
235+
* @param {boolean} allowEmpty Whether the function should fail if no matching nodes can be found.
234236
* @returns {[object]} All the leaf nodes matching the retaining path specification. If none can be found,
235237
* logs the nodes found in the last matching step of the path (if any), and throws an
236238
* assertion error.
237239
*/
238-
function findByRetainingPath(rootName, retainingPath) {
239-
const nodes = createJSHeapSnapshot();
240+
function validateByRetainingPathFromNodes(nodes, rootName, retainingPath, allowEmpty = false) {
240241
let haystack = nodes.filter((n) => n.name === rootName && n.type !== 'string');
241242

242243
for (let i = 0; i < retainingPath.length; ++i) {
@@ -269,6 +270,9 @@ function findByRetainingPath(rootName, retainingPath) {
269270
}
270271

271272
if (newHaystack.length === 0) {
273+
if (allowEmpty) {
274+
return [];
275+
}
272276
const format = (val) => util.inspect(val, { breakLength: 128, depth: 3 });
273277
console.error('#');
274278
console.error('# Retaining path to search for:');
@@ -282,6 +286,7 @@ function findByRetainingPath(rootName, retainingPath) {
282286
}
283287

284288
assert.fail(`Could not find target edge ${format(expected)} in the heap snapshot.`);
289+
285290
}
286291

287292
haystack = newHaystack;
@@ -321,9 +326,19 @@ function getHeapSnapshotOptionTests() {
321326
};
322327
}
323328

329+
/**
330+
* Similar to @see {validateByRetainingPathFromNodes} but creates the snapshot from scratch.
331+
*/
332+
function validateByRetainingPath(...args) {
333+
const nodes = createJSHeapSnapshot();
334+
return validateByRetainingPathFromNodes(nodes, ...args);
335+
}
336+
324337
module.exports = {
325338
recordState,
326339
validateSnapshotNodes,
327-
findByRetainingPath,
340+
validateByRetainingPath,
341+
validateByRetainingPathFromNodes,
328342
getHeapSnapshotOptionTests,
343+
createJSHeapSnapshot,
329344
};

‎test/pummel/test-heapdump-dns.js

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,31 @@
1-
// Flags: --expose-internals
21
'use strict';
2+
// This tests heap snapshot integration of dns ChannelWrap.
3+
34
require('../common');
4-
const { validateSnapshotNodes } = require('../common/heap');
5+
const { validateByRetainingPath } = require('../common/heap');
6+
const assert = require('assert');
7+
8+
// Before dns is loaded, no ChannelWrap should be created.
9+
{
10+
const nodes = validateByRetainingPath('Node / ChannelWrap', []);
11+
assert.strictEqual(nodes.length, 0);
12+
}
513

6-
validateSnapshotNodes('Node / ChannelWrap', []);
714
const dns = require('dns');
8-
validateSnapshotNodes('Node / ChannelWrap', [{}]);
15+
16+
// Right after dns is loaded, a ChannelWrap should be created for the default
17+
// resolver, but it has no task list.
18+
{
19+
validateByRetainingPath('Node / ChannelWrap', [
20+
{ node_name: 'ChannelWrap', edge_name: 'native_to_javascript' },
21+
]);
22+
}
23+
924
dns.resolve('localhost', () => {});
10-
validateSnapshotNodes('Node / ChannelWrap', [
11-
{
12-
children: [
13-
{ node_name: 'Node / NodeAresTask::List', edge_name: 'task_list' },
14-
// `Node / ChannelWrap` (C++) -> `ChannelWrap` (JS)
15-
{ node_name: 'ChannelWrap', edge_name: 'native_to_javascript' },
16-
],
17-
detachedness: 2,
18-
},
19-
]);
25+
26+
// After dns resolution, the ChannelWrap of the default resolver has the task list.
27+
{
28+
validateByRetainingPath('Node / ChannelWrap', [
29+
{ node_name: 'Node / NodeAresTask::List', edge_name: 'task_list' },
30+
]);
31+
}

‎test/pummel/test-heapdump-env.js

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,28 @@
1-
// Flags: --expose-internals
21
'use strict';
32

43
// This tests that Environment is tracked in heap snapshots.
54
// Tests for BaseObject and cppgc-managed objects are done in other
65
// test-heapdump-*.js files.
76

87
require('../common');
9-
const { validateSnapshotNodes } = require('../common/heap');
8+
const { createJSHeapSnapshot, validateByRetainingPathFromNodes } = require('../common/heap');
109

11-
validateSnapshotNodes('Node / Environment', [{
12-
children: [
13-
{ node_name: 'Node / CleanupQueue', edge_name: 'cleanup_queue' },
14-
{ node_name: 'Node / IsolateData', edge_name: 'isolate_data' },
15-
{ node_name: 'Node / PrincipalRealm', edge_name: 'principal_realm' },
16-
],
17-
}]);
10+
const nodes = createJSHeapSnapshot();
1811

19-
validateSnapshotNodes('Node / PrincipalRealm', [{
20-
children: [
21-
{ node_name: 'process', edge_name: 'process_object' },
22-
{ node_name: 'Node / BaseObjectList', edge_name: 'base_object_list' },
23-
],
24-
}]);
12+
const envs = validateByRetainingPathFromNodes(nodes, 'Node / Environment', []);
13+
validateByRetainingPathFromNodes(envs, 'Node / Environment', [
14+
{ node_name: 'Node / CleanupQueue', edge_name: 'cleanup_queue' },
15+
]);
16+
validateByRetainingPathFromNodes(envs, 'Node / Environment', [
17+
{ node_name: 'Node / IsolateData', edge_name: 'isolate_data' },
18+
]);
19+
20+
const realms = validateByRetainingPathFromNodes(envs, 'Node / Environment', [
21+
{ node_name: 'Node / PrincipalRealm', edge_name: 'principal_realm' },
22+
]);
23+
validateByRetainingPathFromNodes(realms, 'Node / PrincipalRealm', [
24+
{ node_name: 'process', edge_name: 'process_object' },
25+
]);
26+
validateByRetainingPathFromNodes(realms, 'Node / PrincipalRealm', [
27+
{ node_name: 'Node / BaseObjectList', edge_name: 'base_object_list' },
28+
]);

‎test/pummel/test-heapdump-fs-promise.js

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,26 @@
1-
// Flags: --expose-internals
21
'use strict';
2+
3+
// This tests heap snapshot integration of fs promise.
4+
35
require('../common');
4-
const { validateSnapshotNodes } = require('../common/heap');
6+
const { validateByRetainingPath, validateByRetainingPathFromNodes } = require('../common/heap');
57
const fs = require('fs').promises;
8+
const assert = require('assert');
9+
10+
// Before fs promise is used, no FSReqPromise should be created.
11+
{
12+
const nodes = validateByRetainingPath('Node / FSReqPromise', []);
13+
assert.strictEqual(nodes.length, 0);
14+
}
615

7-
validateSnapshotNodes('Node / FSReqPromise', []);
816
fs.stat(__filename);
9-
validateSnapshotNodes('Node / FSReqPromise', [
10-
{
11-
children: [
12-
{ node_name: 'FSReqPromise', edge_name: 'native_to_javascript' },
13-
{ node_name: 'Node / AliasedFloat64Array', edge_name: 'stats_field_array' },
14-
],
15-
},
16-
]);
17+
18+
{
19+
const nodes = validateByRetainingPath('Node / FSReqPromise', []);
20+
validateByRetainingPathFromNodes(nodes, 'Node / FSReqPromise', [
21+
{ node_name: 'FSReqPromise', edge_name: 'native_to_javascript' },
22+
]);
23+
validateByRetainingPathFromNodes(nodes, 'Node / FSReqPromise', [
24+
{ node_name: 'Node / AliasedFloat64Array', edge_name: 'stats_field_array' },
25+
]);
26+
}

‎test/pummel/test-heapdump-http2.js

Lines changed: 43 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,21 @@
1-
// Flags: --expose-internals
21
'use strict';
2+
3+
// This tests heap snapshot integration of http2.
4+
35
const common = require('../common');
4-
const { recordState } = require('../common/heap');
6+
const { createJSHeapSnapshot, validateByRetainingPath, validateByRetainingPathFromNodes } = require('../common/heap');
7+
const assert = require('assert');
8+
59
if (!common.hasCrypto)
610
common.skip('missing crypto');
711
const http2 = require('http2');
812

13+
// Before http2 is used, no Http2Session or Http2Streamshould be created.
914
{
10-
const state = recordState();
11-
state.validateSnapshotNodes('Node / Http2Session', []);
12-
state.validateSnapshotNodes('Node / Http2Stream', []);
15+
const sessions = validateByRetainingPath('Node / Http2Session', []);
16+
assert.strictEqual(sessions.length, 0);
17+
const streams = validateByRetainingPath('Node / Http2Stream', []);
18+
assert.strictEqual(streams.length, 0);
1319
}
1420

1521
const server = http2.createServer();
@@ -21,63 +27,45 @@ server.listen(0, () => {
2127
const req = client.request();
2228

2329
req.on('response', common.mustCall(() => {
24-
const state = recordState();
30+
const nodes = createJSHeapSnapshot();
2531

2632
// `Node / Http2Stream` (C++) -> Http2Stream (JS)
27-
state.validateSnapshotNodes('Node / Http2Stream', [
28-
{
29-
children: [
30-
// current_headers and/or queue could be empty
31-
{ node_name: 'Http2Stream', edge_name: 'native_to_javascript' },
32-
],
33-
},
34-
], { loose: true });
33+
validateByRetainingPathFromNodes(nodes, 'Node / Http2Stream', [
34+
// current_headers and/or queue could be empty
35+
{ node_name: 'Http2Stream', edge_name: 'native_to_javascript' },
36+
]);
3537

3638
// `Node / FileHandle` (C++) -> FileHandle (JS)
37-
state.validateSnapshotNodes('Node / FileHandle', [
38-
{
39-
children: [
40-
{ node_name: 'FileHandle', edge_name: 'native_to_javascript' },
41-
// current_headers could be empty
42-
],
43-
},
44-
], { loose: true });
45-
state.validateSnapshotNodes('Node / TCPSocketWrap', [
46-
{
47-
children: [
48-
{ node_name: 'TCP', edge_name: 'native_to_javascript' },
49-
],
50-
},
51-
], { loose: true });
52-
state.validateSnapshotNodes('Node / TCPServerWrap', [
53-
{
54-
children: [
55-
{ node_name: 'TCP', edge_name: 'native_to_javascript' },
56-
],
57-
},
58-
], { loose: true });
39+
validateByRetainingPathFromNodes(nodes, 'Node / FileHandle', [
40+
{ node_name: 'FileHandle', edge_name: 'native_to_javascript' },
41+
// current_headers could be empty
42+
]);
43+
validateByRetainingPathFromNodes(nodes, 'Node / TCPSocketWrap', [
44+
{ node_name: 'TCP', edge_name: 'native_to_javascript' },
45+
]);
46+
47+
validateByRetainingPathFromNodes(nodes, 'Node / TCPServerWrap', [
48+
{ node_name: 'TCP', edge_name: 'native_to_javascript' },
49+
]);
50+
5951
// `Node / StreamPipe` (C++) -> StreamPipe (JS)
60-
state.validateSnapshotNodes('Node / StreamPipe', [
61-
{
62-
children: [
63-
{ node_name: 'StreamPipe', edge_name: 'native_to_javascript' },
64-
],
65-
},
52+
validateByRetainingPathFromNodes(nodes, 'Node / StreamPipe', [
53+
{ node_name: 'StreamPipe', edge_name: 'native_to_javascript' },
6654
]);
55+
6756
// `Node / Http2Session` (C++) -> Http2Session (JS)
68-
state.validateSnapshotNodes('Node / Http2Session', [
69-
{
70-
children: [
71-
{ node_name: 'Http2Session', edge_name: 'native_to_javascript' },
72-
{ node_name: 'Node / nghttp2_memory', edge_name: 'nghttp2_memory' },
73-
{
74-
node_name: 'Node / streams', edge_name: 'streams',
75-
},
76-
// outstanding_pings, outgoing_buffers, outgoing_storage,
77-
// pending_rst_streams could be empty
78-
],
79-
},
80-
], { loose: true });
57+
const sessions = validateByRetainingPathFromNodes(nodes, 'Node / Http2Session', []);
58+
validateByRetainingPathFromNodes(sessions, 'Node / Http2Session', [
59+
{ node_name: 'Http2Session', edge_name: 'native_to_javascript' },
60+
]);
61+
validateByRetainingPathFromNodes(sessions, 'Node / Http2Session', [
62+
{ node_name: 'Node / nghttp2_memory', edge_name: 'nghttp2_memory' },
63+
]);
64+
validateByRetainingPathFromNodes(sessions, 'Node / Http2Session', [
65+
{ node_name: 'Node / streams', edge_name: 'streams' },
66+
]);
67+
// outstanding_pings, outgoing_buffers, outgoing_storage,
68+
// pending_rst_streams could be empty
8169
}));
8270

8371
req.resume();

‎test/pummel/test-heapdump-inspector.js

Lines changed: 16 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,29 @@
1-
// Flags: --expose-internals
21
'use strict';
3-
const common = require('../common');
2+
// This tests heap snapshot integration of inspector.
43

4+
const common = require('../common');
5+
const assert = require('assert');
56
common.skipIfInspectorDisabled();
67

7-
const { validateSnapshotNodes } = require('../common/heap');
8+
const { validateByRetainingPath, validateByRetainingPathFromNodes } = require('../common/heap');
89
const inspector = require('inspector');
910

10-
const snapshotNode = {
11-
children: [
12-
{ node_name: 'Node / InspectorSession', edge_name: 'session' },
13-
],
14-
};
15-
16-
// Starts with no JSBindingsConnection (or 1 if coverage enabled).
11+
// Starts with no JSBindingsConnection.
1712
{
18-
const expected = [];
19-
if (process.env.NODE_V8_COVERAGE) {
20-
expected.push(snapshotNode);
21-
}
22-
validateSnapshotNodes('Node / JSBindingsConnection', expected);
13+
const nodes = validateByRetainingPath('Node / JSBindingsConnection', []);
14+
assert.strictEqual(nodes.length, 0);
2315
}
2416

25-
// JSBindingsConnection should be added.
17+
// JSBindingsConnection should be added once inspector session is created.
2618
{
2719
const session = new inspector.Session();
2820
session.connect();
29-
const expected = [
30-
{
31-
children: [
32-
{ node_name: 'Node / InspectorSession', edge_name: 'session' },
33-
{ node_name: 'Connection', edge_name: 'native_to_javascript' },
34-
(edge) => edge.name === 'callback' &&
35-
(edge.to.type === undefined || // embedded graph
36-
edge.to.type === 'closure'), // snapshot
37-
],
38-
},
39-
];
40-
if (process.env.NODE_V8_COVERAGE) {
41-
expected.push(snapshotNode);
42-
}
43-
validateSnapshotNodes('Node / JSBindingsConnection', expected);
21+
22+
const nodes = validateByRetainingPath('Node / JSBindingsConnection', []);
23+
validateByRetainingPathFromNodes(nodes, 'Node / JSBindingsConnection', [
24+
{ node_name: 'Node / InspectorSession', edge_name: 'session' },
25+
]);
26+
validateByRetainingPathFromNodes(nodes, 'Node / JSBindingsConnection', [
27+
{ node_name: 'Connection', edge_name: 'native_to_javascript' },
28+
]);
4429
}

0 commit comments

Comments
 (0)