Strict mocking isn't working as expected

[gmahomarf]

I've noticed two problems with strict mocking:

1. Given a dependency that provides a default export, when I call esmock in strict mode on a dependent module and I don't define a default export for the mocked dependency, I expect esmock to fail due to the missing default export. What actually happens is that it sets the entire mock definition object to the value of the default export.
2. When I call esmock in strict mode on a module and I don't include a mock of one of its dependencies, I expect esmock to fail due to trying to import a missing dependency. What actually happens is that the original module is imported, and production code is executed (I would expect this behavior in partial mocking)

I've made an example gist: https://gist.github.com/gmahomarf/c60e05ef57c35aad8e18590df8e3f945

[iambumblehead]

For the first issue, in order to support "short-hand" default exports, esmock does currently apply the mock definition object as named and default exports. Maybe this behaviour should be disabled for strict mode, when the mock definition is an object.

For the second issue, this is the intended behaviour,

• if a module's specifier is not found in the mock definition, the original module continues to be imported to the mock import tree,
• if a module's specifier is found in the mock definition of a strict import tree, only the mock definition is imported to the mock import tree,
• if a module's specifier is found in the mock definition of a partial import tree, the mocked definition and original definition are merged and the result is imported to the import tree

[gmahomarf]

Thanks for the quick response.

I had noticed the short-hand default imports that you mentioned, but I can't really say what would be the right approach here. It's a pretty useful feature for both strict and partial modes, and I'm not sure it would be easy to distinguish between a short-hand mock and one that's just missing the default value.

For the second issue, any chance I can convince you to change the behavior of strict mode so that it doesn't import anything that isn't explicitly defined in the mock definition? Or maybe add a stricter version that does this?

[iambumblehead]

We could update esmock to use the "very strict" behaviour described in point 2. If we do this, my preference is to introduce a new mode for this like "esmock.strictest". Do you think these would be OK @gmahomarf?

1. add support for esmock.strictest --require all mocked tree imports to be defined,
2. disable short-hand default for strict mode (or maybe for strictest mode only). If a mock definition is an object and no default definition is defined, do not define the mock definition as the default export of the specified module

[gmahomarf]

1. 👍
2. I think this one merits a larger discussion with a larger audience. On the one hand, it's a pretty useful feature. On the other hand, default exports are always explicitly defined in real modules, so it makes sense they would have to be explicitly defined in mock modules.

[iambumblehead]

@gmahomarf if an imported module is not mocked for the strictest import tree, which outcome do you prefer?

1. the imported module exports something like export { nullval as default } and a runtime error will occur when the parent module attempts to use the imported child,
   • "child.namedExport is not a function"
2. the imported module exports nothing and a runtime error like this would occur,
   • "the requested module './child.js' does not provide an export named 'default'"
3. esmock throws a runtime error like this,
   • "un-mocked moduleId: '~/root/child.js' (used by ~/root/parent.js)"

The third case provides a clear error message, but would require ALL imported modules to be mocked at the parent. The second case would only require imported modules to be mocked when they are actually used by the parent in the test.

Let's use the one you think is best and, in the meantime, get a PR ready using maybe the third option and we'll update it however you want.

[iambumblehead]

@gmahomarf to support the first option completely, named exports at the child module would need to be defined by esmock so as to support import { name } from './child.js' otherwise runtime errors occur right away before the child module is used by the parent.

Supporting the second and third option above is preferred because those are simpler and require fewer additions.

[iambumblehead]

@tripodsan @aladdin-add feel free to ignore this of course but if you have an opinion on either of these it would be great to know what you think.

[gmahomarf]

If I'm not mistaken, options one and two already happen in strict mode (you can see option two in action in my gist, the test under 'named export missing')

For strictest, I'm leaning towards option 3. It makes the most sense to me.

[iambumblehead]

@gmahomarf cool 👍 if the PR looks good to you and if you want it published now we could do that, otherwise we could wait a day or so and maybe get review from @tripodsan

[gmahomarf]

I'm in no hurry.

EDIT: PR looks good

[iambumblehead]

its published https://github.com/iambumblehead/esmock/releases/tag/v2.0.5