Test runner module mocking format

[GeoffreyBooth]

Spun off from #53619 (comment); I wanted to start a new issue for this as it’s arguably a separate issue from unflagging --experimental-detect-module. cc @cjihrig @nodejs/test_runner

Regarding the module mocking - the mock loader (lib/test/mock_loader.js) uses that information to determine whether to generate source code in CJS or ESM. I think there are a few options to fix it:

• Maybe it doesn't really matter and it can be removed?
• Pick a default when the hint is undefined.
• Make it an option that the user can provide as input.

It seems like @cjihrig above is referring to this? https://nodejs.org/docs/latest/api/test.html#mockmodulespecifier-options. Which seems to be implemented here?

node/lib/internal/test_runner/mock/mock.js

Lines 489 to 516 in 02bd866

	module(specifier, options = kEmptyObject) {
	emitExperimentalWarning('Module mocking');
	validateString(specifier, 'specifier');
	validateObject(options, 'options');
	debug('module mock entry, specifier = "%s", options = %o', specifier, options);
	const {
	cache = false,
	namedExports = kEmptyObject,
	defaultExport,
	} = options;
	const hasDefaultExport = 'defaultExport' in options;
	validateBoolean(cache, 'options.cache');
	validateObject(namedExports, 'options.namedExports');
	const sharedState = setupSharedModuleState();
	const mockSpecifier = StringPrototypeStartsWith(specifier, 'node:') ?
	StringPrototypeSlice(specifier, 5) : specifier;
	// Get the file that called this function. We need four stack frames:
	// vm context -> getStructuredStack() -> this function -> actual caller.
	const caller = getStructuredStack()[3]?.getFileName();
	const { format, url } = sharedState.moduleLoader.resolveSync(
	mockSpecifier, caller, null,
	);
	debug('module mock, url = "%s", format = "%s", caller = "%s"', url, format, caller);
	validateOneOf(format, 'format', kSupportedFormats);

Per the docs, mock.module supports defining mocks for both module systems based on passed-in objects (not source code). Then we generate source code here?

node/lib/test/mock_loader.js

Lines 163 to 175 in 02bd866

	async function createSourceFromMock(mock) {
	// Create mock implementation from provided exports.
	const { exportNames, format, hasDefaultExport, url } = mock;
	const useESM = format === 'module';
	const source = `${testImportSource(useESM)}
	if (!$__test.mock._mockExports.has('${url}')) {
	throw new Error(${JSONStringify(`mock exports not found for "${url}"`)});
	}
	const $__exports = $__test.mock._mockExports.get(${JSONStringify(url)});
	${defaultExportSource(useESM, hasDefaultExport)}
	${namedExportsSource(useESM, exportNames)}
	`;

At first I was confused as to how this works, since ESM is unlike CommonJS in that ESM can have a default export that’s something other than an object whose properties are the named exports; so how can this one API generate mocks that apply to both module systems while still supporting this ESM-only feature. But then I noticed that an exception gets thrown if the mock is created with both defaultExport and namedExports defined and the module is required. So I guess really it supports both module systems only if you give it input that can be consumed by both systems.

But maybe that’s the answer: if the user defines one of defaultExport or namedExports, it creates the source as CommonJS, and either maps the provided defaultExport to module.exports or it adds each of the namedExports onto module.exports. If the user defines both defaultExport and namedExports, the mock is created as ESM and the restrictions mentioned in the docs apply.

Alternatively the mock could always be created as ESM, though then you have the issue that for it to be require-able --experimental-require-module needs to be enabled. But maybe that behavior could be opted into by using this API. Since the user is passing in an object I assume we don’t need to worry about top-level await.

Or we could just ask the user what format to generate the source code in. We kind of already are, though the users probably don’t realize it, since we ask for the URL of the module to be mocked and base the format on that. Though it seems like you’re trying to keep this as simple as possible so if there’s a way to avoid adding another option, I assume you’d prefer to determine this implicitly. I think determining format based on defaultExport / namedExports is a better source to use rather than how the file URL resolves.

[cjihrig]

I think the simplest way to solve this would be to use the hint if present. If the hint is not present, use some default. To be more powerful/flexible, we could add another option to the API that lets the user be explicit about the format they want.

[GeoffreyBooth]

I think the simplest way to solve this would be to use the hint if present. If the hint is not present, use some default. To be more powerful/flexible, we could add another option to the API that lets the user be explicit about the format they want.

What does this mean in practice? If resolve returns format use it, and if not, use module if the user has defined both defaultExport and namedExport; or use commonjs otherwise?

I think rather than having the docs explain how the hint is determined based on URL and when it might not exist and what happens in that case, et cetera, a simpler approach could be explained in the docs as follows:

When only one of defaultExport or namedExports is defined, the mock will be created as a CommonJS module so that it can be used via either require or import. If both defaultExport and namedExports are defined, the mock will be created as an ES module that can be accessed via import, and also via require if --experiemental-require-module is passed.

[cjihrig]

use module if the user has defined both defaultExport and namedExport

I would avoid using this heuristic because defaultExport and namedExports are valid and useful for many CJS use cases.

How many cases in the current module mocking tests begin to fail with the changes in #53619? If it is a lot, then I think we may want to figure out an appropriate default (maybe 'commonjs' as suggested in #53619 (comment)). If it is one or two cases, then maybe we keep the default behavior of throwing when there is no known format and just require users to pass an option in those cases.

[GeoffreyBooth]

How many cases in the current module mocking tests begin to fail with the changes in #53619?

In ./node --experimental-test-module-mocks --experimental-require-module ./test/parallel/test-runner-module-mocking.js the following fail:

• CJS mocking with namedExports option
  • does not cache by default
  • explicitly enables caching
  • explicitly disables caching
  • named exports are applied to defaultExport
  • throws if named exports cannot be applied to defaultExport
• CJS mocking with namedExports option
  • throws if named exports cannot be applied to defaultExport as CJS
• ESM mocking with namedExports option
  • CJS
• modules cannot be mocked multiple times at once
  • CJS
  • mocks are automatically restored
• mocks can be restored independently
  • CJS mocks can be used by both module systems
  • node_modules can be used by both module systems
  • mocked modules do not impact unmocked modules
  • defaultExports work with CJS mocks in both module systems

Basically any test where the mock URL is referring to a .js file where there’s no package.json with a type field, since for all these cases the format is undefined and the mocking code has an assertion that format be one of builtin, commonjs or module.

Since there’s no file at the URL provided, I’m not sure why we’re using anything about the URL as part of determining format. That part feels wrong to me.

I would avoid using this heuristic because defaultExport and namedExports are valid and useful for many CJS use cases.

Because of the time when they can be used together, if the default export happens to be an object with properties that match the provided named exports?

It seems to me like the most convenient API would be one where the sources are always generated as ESM and --experimental-require-module is enabled automatically when this API is used, at least for requiring these mocks. If that feels too risky, then sure, asking the user to specify the format is better than guessing based on a URL of a file that doesn’t exist.

[GeoffreyBooth]

Actually I might’ve been mistaken about one part: does the URL of the module to be mocked need to exist? Because if we only support mocking modules that exist, we can use nextLoad to get the format, and then the API doesn’t need to change. It’s only if we support mocking URLs of nonexistent modules that we need some new way of determining the format.

#53642