test: improve test-bootstrap-modules.js

joyeecheung · RafaelGSS · commit 4b26f14a5310 · 2023-11-29T09:45:22.000-03:00
Divide builtins into two lists depending on whether they are loaded before pre-execution or at run time, and give clearer suggestions about how to deal with them based on the category they are in. This helps preventing regressions like the one reported in #45662. PR-URL: #50708 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js
@@ -1,14 +1,27 @@
-// Flags: --expose-internals
 'use strict';
 
-// This list must be computed before we require any modules to
+// This list must be computed before we require any builtins to
 // to eliminate the noise.
-const actualModules = new Set(process.moduleLoadList.slice());
+const list = process.moduleLoadList.slice();
 
 const common = require('../common');
 const assert = require('assert');
+const { inspect } = require('util');
 
-const expectedModules = new Set([
+const preExecIndex =
+  list.findIndex((i) => i.includes('pre_execution'));
+const actual = {
+  beforePreExec: new Set(list.slice(0, preExecIndex)),
+  atRunTime: new Set(list.slice(preExecIndex)),
+};
+
+// Currently, we don't add additional builtins to worker snapshots.
+// So for worker snapshots we'll just concatenate the two. Once we
+// add more builtins to worker snapshots, we should also distinguish
+// the two stages for them.
+const expected = {};
+
+expected.beforePreExec = new Set([
   'Internal Binding builtins',
   'Internal Binding encoding_binding',
   'Internal Binding errors',
@@ -84,22 +97,25 @@ const expectedModules = new Set([
   'NativeModule internal/modules/package_json_reader',
   'Internal Binding module_wrap',
   'NativeModule internal/modules/cjs/loader',
-  'NativeModule internal/vm/module',
-  'NativeModule internal/modules/esm/utils',
+]);
+
+expected.atRunTime = new Set([
   'Internal Binding wasm_web_api',
   'Internal Binding worker',
   'NativeModule internal/modules/run_main',
   'NativeModule internal/net',
   'NativeModule internal/dns/utils',
   'NativeModule internal/process/pre_execution',
+  'NativeModule internal/vm/module',
+  'NativeModule internal/modules/esm/utils',
 ]);
 
 if (common.isMainThread) {
   [
     'NativeModule internal/idna',
     'NativeModule url',
-  ].forEach(expectedModules.add.bind(expectedModules));
-} else {
+  ].forEach(expected.beforePreExec.add.bind(expected.beforePreExec));
+} else {  // Worker.
   [
     'NativeModule diagnostics_channel',
     'NativeModule internal/abort_controller',
@@ -127,35 +143,97 @@ if (common.isMainThread) {
     'NativeModule stream/promises',
     'NativeModule string_decoder',
     'NativeModule worker_threads',
-  ].forEach(expectedModules.add.bind(expectedModules));
+  ].forEach(expected.atRunTime.add.bind(expected.atRunTime));
+  // For now we'll concatenate the two stages for workers. We prefer
+  // atRunTime here because that's what currently happens for these.
 }
 
 if (common.isWindows) {
   // On Windows fs needs SideEffectFreeRegExpPrototypeExec which uses vm.
-  expectedModules.add('NativeModule vm');
+  expected.atRunTime.add('NativeModule vm');
 }
 
 if (common.hasIntl) {
-  expectedModules.add('Internal Binding icu');
+  expected.beforePreExec.add('Internal Binding icu');
 }
 
 if (process.features.inspector) {
-  expectedModules.add('Internal Binding inspector');
-  expectedModules.add('NativeModule internal/inspector_async_hook');
-  expectedModules.add('NativeModule internal/util/inspector');
+  expected.beforePreExec.add('Internal Binding inspector');
+  expected.beforePreExec.add('NativeModule internal/util/inspector');
+  expected.atRunTime.add('NativeModule internal/inspector_async_hook');
 }
 
 const difference = (setA, setB) => {
   return new Set([...setA].filter((x) => !setB.has(x)));
 };
-const missingModules = difference(expectedModules, actualModules);
-const extraModules = difference(actualModules, expectedModules);
-const printSet = (s) => { return `${[...s].sort().join(',\n  ')}\n`; };
-
-assert.deepStrictEqual(actualModules, expectedModules,
-                       (missingModules.size > 0 ?
-                         'These modules were not loaded:\n  ' +
-                         printSet(missingModules) : '') +
-                       (extraModules.size > 0 ?
-                         'These modules were unexpectedly loaded:\n  ' +
-                         printSet(extraModules) : ''));
+
+// Accumulate all the errors and print them at the end instead of throwing
+// immediately which makes it harder to update the test.
+const errorLogs = [];
+function err(message) {
+  if (typeof message === 'string') {
+    errorLogs.push(message);
+  } else {
+    // Show the items in individual lines for easier copy-pasting.
+    errorLogs.push(inspect(message, { compact: false }));
+  }
+}
+
+if (common.isMainThread) {
+  const missing = difference(expected.beforePreExec, actual.beforePreExec);
+  const extra = difference(actual.beforePreExec, expected.beforePreExec);
+  if (missing.size !== 0) {
+    err('These builtins are now no longer loaded before pre-execution.');
+    err('If this is intentional, remove them from `expected.beforePreExec`.');
+    err('\n--- These could be removed from expected.beforePreExec ---');
+    err([...missing].sort());
+    err('');
+  }
+  if (extra.size !== 0) {
+    err('These builtins are now unexpectedly loaded before pre-execution.');
+    err('If this is intentional, add them to `expected.beforePreExec`.');
+    err('\n# Note: loading more builtins before pre-execution can lead to ' +
+        'startup performance regression or invalid snapshots.');
+    err('- Consider lazy loading builtins that are not used universally.');
+    err('- Make sure that the builtins do not access environment dependent ' +
+        'states e.g. command line arguments or environment variables ' +
+        'during loading.');
+    err('- When in doubt, ask @nodejs/startup.');
+    err('\n--- These could be added to expected.beforePreExec ---');
+    err([...extra].sort());
+    err('');
+  }
+}
+
+if (!common.isMainThread) {
+  // For workers, just merge beforePreExec into atRunTime for now.
+  // When we start adding modules to the worker snapshot, this branch
+  // can be removed and  we can just remove the common.isMainThread
+  // conditions.
+  expected.beforePreExec.forEach(expected.atRunTime.add.bind(expected.atRunTime));
+  actual.beforePreExec.forEach(actual.atRunTime.add.bind(actual.atRunTime));
+}
+
+{
+  const missing = difference(expected.atRunTime, actual.atRunTime);
+  const extra = difference(actual.atRunTime, expected.atRunTime);
+  if (missing.size !== 0) {
+    err('These builtins are now no longer loaded at run time.');
+    err('If this is intentional, remove them from `expected.atRunTime`.');
+    err('\n--- These could be removed from expected.atRunTime ---');
+    err([...missing].sort());
+    err('');
+  }
+  if (extra.size !== 0) {
+    err('These builtins are now unexpectedly loaded at run time.');
+    err('If this is intentional, add them to `expected.atRunTime`.');
+    err('\n# Note: loading more builtins at run time can lead to ' +
+        'startup performance regression.');
+    err('- Consider lazy loading builtins that are not used universally.');
+    err('\n--- These could be added to expected.atRunTime ---');
+    err([...extra].sort());
+    err('');
+  }
+}
+
+assert.strictEqual(errorLogs.length, 0, errorLogs.join('\n'));
