test: improve test-bootstrap-modules.js

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.

This helps preventing regressions like the one reported
in nodejs#45662.

test/parallel/test-bootstrap-modules.js

@@ -1,14 +1,28 @@
-// 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 { writeSync } = require('fs');
+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, list.length)),
+};
+
+// 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 +98,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 +144,91 @@ 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) : ''));
+
+const reasons = [];
+function err(message) {
+  reasons.push(typeof message === 'string' ? message : inspect(message));
+}
+
+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('');
+    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('Note: loading more builtins before pre-execution can lead to ' +
+        'startup performance regression or invalid snapshots.');
+    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('');
+    err([...extra].sort());
+    err('');
+  }
+}
+
+if (!common.isMainThread) {
+  // For worker, 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('');
+    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('Note: loading more builtins before pre-execution can lead to ' +
+        'startup performance regression. Consider lazy loading builtins' +
+        'that are not commonly used.');
+    err('');
+    err([...extra].sort());
+    err('');
+  }
+}
+
+if (reasons.length > 0) {
+  throw new Error(reasons.join('\n'));
+}