Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ESM test regression when run under quibble@0.6.10 #493

Closed
searls opened this issue Jul 14, 2022 · 4 comments
Closed

ESM test regression when run under quibble@0.6.10 #493

searls opened this issue Jul 14, 2022 · 4 comments

Comments

@searls
Copy link
Member

searls commented Jul 14, 2022

Tagging in @connorjclark if he'd be willing to look at these test failures:

After updating to quibble@0.6.10 (this PR testdouble/quibble#72 ), npm run test:esm is failing in testdouble.js:

$ npm run test:esm

> testdouble@3.16.6 test:esm
> if node test/supports-esm.js; then NODE_OPTIONS='--loader=quibble' TS_NODE_IGNORE='(?:^|/)node_modules/,notypescript' ./test/safe-esm/teenytest-proxy.js --helper test/helper.js test/safe-esm/replace.test.js; fi

(node:79591) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:79591) DeprecationWarning: Obsolete loader hook(s) supplied and will be ignored: getFormat, getSource
TAP version 13
1..9
ok 1 - "Node.js-specific ESM module replacement quibbling prototypal constructors get created with td.object(Type)" - test #1 in `test/safe-esm/replace.test.js`
ok 2 - "Node.js-specific ESM module replacement quibbling plain old functions with td.function()" - test #2 in `test/safe-esm/replace.test.js`
ok 3 - "Node.js-specific ESM module replacement naming the doubles of functions with names" - test #3 in `test/safe-esm/replace.test.js`
ok 4 - "Node.js-specific ESM module replacement faking property on exported function" - test #4 in `test/safe-esm/replace.test.js`
ok 5 - "Node.js-specific ESM module replacement manually stubbing an entry" - test #5 in `test/safe-esm/replace.test.js`
ok 6 - "Node.js-specific ESM module replacement an object of funcs" - test #6 in `test/safe-esm/replace.test.js`
not ok 7 - "Node.js-specific ESM module replacement and classes on objects on funcs" - test #7 in `test/safe-esm/replace.test.js`
  ---
  Error: ERROR: lodash _.isEqual assertion failed.

Expected value:
"yow"

Actual value:
undefined

    at Function.assert._isEqual (/Users/justin/code/testdouble/testdouble.js/test/support/custom-assertions.js:8:13)
    at Object.and classes on objects on funcs (/Users/justin/code/testdouble/testdouble.js/test/safe-esm/replace.test.js:48:14)
    at /Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/callbackify.js:14:21
    at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
    at Object.userFunction [as wrap] (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest-promise/index.js:5:7)
    at callable (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:29:24)
    at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
    at Object.userFunction [as wrap] (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/plugins/uncaught-exception.js:16:9)
    at callable (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:29:24)
    at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
  ...
not ok 8 - "Node.js-specific ESM module replacement faking a 3rd party module" - test #8 in `test/safe-esm/replace.test.js`
  ---
  Error: ERROR: lodash _.isEqual assertion failed.

Expected value:
true

Actual value:
undefined

    at Function.assert._isEqual (/Users/justin/code/testdouble/testdouble.js/test/support/custom-assertions.js:8:13)
    at Object.faking a 3rd party module (/Users/justin/code/testdouble/testdouble.js/test/safe-esm/replace.test.js:53:14)
    at /Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/callbackify.js:14:21
    at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
    at Object.userFunction [as wrap] (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest-promise/index.js:5:7)
    at callable (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:29:24)
    at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
    at Object.userFunction [as wrap] (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/plugins/uncaught-exception.js:16:9)
    at callable (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:29:24)
    at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
  ...
not ok 9 - "Node.js-specific ESM module replacement post-reset usage" - test #9 in `test/safe-esm/replace.test.js`
  ---
  Error: ERROR: lodash _.isEqual assertion failed.

Expected value:
"Cannot find module "

Actual value:
"should have errored!"

    at Function.assert._isEqual (/Users/justin/code/testdouble/testdouble.js/test/support/custom-assertions.js:8:13)
    at Object.post-reset usage (/Users/justin/code/testdouble/testdouble.js/test/safe-esm/replace.test.js:62:16)
  ...
# Test run failed!
#   Passed: 6
#   Failed: 3
#   Total:  9
#
# Failures:
#
#   7 - "Node.js-specific ESM module replacement and classes on objects on funcs" - test #7 in `test/safe-esm/replace.test.js`
#
#     Error: ERROR: lodash _.isEqual assertion failed.
#     
#     Expected value:
#     "yow"
#     
#     Actual value:
#     undefined
#     
#         at Function.assert._isEqual (/Users/justin/code/testdouble/testdouble.js/test/support/custom-assertions.js:8:13)
#         at Object.and classes on objects on funcs (/Users/justin/code/testdouble/testdouble.js/test/safe-esm/replace.test.js:48:14)
#         at /Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/callbackify.js:14:21
#         at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
#         at Object.userFunction [as wrap] (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest-promise/index.js:5:7)
#         at callable (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:29:24)
#         at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
#         at Object.userFunction [as wrap] (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/plugins/uncaught-exception.js:16:9)
#         at callable (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:29:24)
#         at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
#
#   8 - "Node.js-specific ESM module replacement faking a 3rd party module" - test #8 in `test/safe-esm/replace.test.js`
#
#     Error: ERROR: lodash _.isEqual assertion failed.
#     
#     Expected value:
#     true
#     
#     Actual value:
#     undefined
#     
#         at Function.assert._isEqual (/Users/justin/code/testdouble/testdouble.js/test/support/custom-assertions.js:8:13)
#         at Object.faking a 3rd party module (/Users/justin/code/testdouble/testdouble.js/test/safe-esm/replace.test.js:53:14)
#         at /Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/callbackify.js:14:21
#         at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
#         at Object.userFunction [as wrap] (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest-promise/index.js:5:7)
#         at callable (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:29:24)
#         at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
#         at Object.userFunction [as wrap] (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/plugins/uncaught-exception.js:16:9)
#         at callable (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:29:24)
#         at runX (/Users/justin/code/testdouble/testdouble.js/node_modules/teenytest/lib/plugins/wrap.js:22:7)
#
#   9 - "Node.js-specific ESM module replacement post-reset usage" - test #9 in `test/safe-esm/replace.test.js`
#
#     Error: ERROR: lodash _.isEqual assertion failed.
#     
#     Expected value:
#     "Cannot find module "
#     
#     Actual value:
#     "should have errored!"
#     
#         at Function.assert._isEqual (/Users/justin/code/testdouble/testdouble.js/test/support/custom-assertions.js:8:13)
#         at Object.post-reset usage (/Users/justin/code/testdouble/testdouble.js/test/safe-esm/replace.test.js:62:16)
@searls searls mentioned this issue Jul 14, 2022
Closed
@connorjclark
Copy link

connorjclark commented Jul 14, 2022 •
edited

TL;DR: please revert that change, we'll need some other approach.

In hindsight, the test here testdouble/quibble@5a83416#diff-be23e0c4db6635e4d0ad178cee88625670d2e51b6844c436f5b206c1fe5279bdR121 was flawed.

The test lays out a reasonable desired property of the mocking library:

module graph: A imports B
import B as B1, then mock A, then import B again as B2
B1 === B2

but there's also the equally desirable property that if an un-mocked module itself imports a mocked module, future imports of that module should use the mocked version. This requires reseting the module cache ... in direct conflict to the previous case.

module graph: A imports B
import A as A1, then mock B, then import A again as A2
A2 should hold references to the mocked B, which necessarily means A1 !== A2 (cache was busted)

My recent change to quibble broke this second property.

Aside: Ideally, one could "hot-swap" existing static module references in other modules but... I don't think that is possible, given Module objects are read-only. Besides, if a static import is used in the root module scope, that code would not be refreshed as expected. But if it were possible that seems like an OK constraint to have on modules-under-test.

The cache busting mechanism controlling this behavior is stubModuleGeneration, which is increased with every call to quibble.esm (and survives resets, because otherwise the library would continue loading cached modules after a reset...). My recent change circumvented this behavior for modules that aren't being mocked... which broke the second case above. In hindsight, I think the proper fix would have been what I explained here.

My initial thoughts are that this problem is truly cursed. My second thoughts are that perhaps proper mocking of ES modules is still possible, but only correctly when all calls to td.replaceEsm/quibble.esm are done before importing any part of the module graph lest you run into the issue that prompted my PR (that is, hard-to-debug problems with class/symbol equality)

It could be enforced with an optional strict configuration in quibble:

  • In the loader, for every module (not just ones being mocked) record what the stubModuleGeneration value is in strictCache
  • If asked to load the same module again but stubModuleGeneration is now higher, throw an error
  • On reset, clear strictCache

This would basically "freeze" the module graph after every series of calls to td.replaceEsm, making sure that the pre-mocked graph does not bleed into the mocked graph. It would need to be accompanied by the wrapper-file idea for ease of use (otherwise you'll have dynamic import hell and need to think hard about your module graph) – but such a mode would have alerted me quickly that something was amiss, instead of having to debug what myClass instanceof MyClass is failing :P

@searls
Copy link
Member Author

searls commented Jul 14, 2022

Thanks @connorjclark! Tagging in @giltayar because he understands this better than I do.

@giltayar
Copy link
Collaborator

OK, having gone the whole route (implementing it my way, and then seeing that implementing it @connorjclark's way is simpler (although a few tweaks to the code are necessary), I think I understand the issues better now.

Yes, we're in a "damned if we do, damned if we don't" situation here. BTW, this situation is not specific to ESM, and is relevant to any module loading/mocking situation.

The question @connorjclark raised is this: if I mock a module B used by module A (which is unmocked), if I remock B and import A, should it make A load the newly mocked B, or stay with the original mock?

If we go with the former, this means that A is reloaded and becomes A' (i.e. statics will change). If we go with the latter, the user is surprised that B is not mocked.

From my point of view, the latter behavior, where the user is surprised that no mocking occurs even if mocking is requested, is more problematic than the latter. We should leave it at that! Yes, at the cost of module state being difference between imports.

Your "strict" mode is an interesting idea that helps debug module-state-being-unequal.issues, but my guess is that most users won't care if there is reloading of modules and so won't need the "strict" mode. And if they do care, I'm not sure that they would know to turn it on and make it help them figure out what the problem is!

First off, whatever the result we decide, there's needs to be a test for that, so that we will document this behavior :-)

@searls what do you think? There are two behaviors, each having their own pros and cons:

The scenario is this:

A is a module that imports B

Now the code:

import {func} from A
mock B
import {func as func2} from A

  1. The current behavior (before the problematic PR): A is reimported, and the result uses a mocked B. But A is reimported, so func and func2 are not equal (functions are references).
  2. The new behavior (from this PR): A is not reimported, and the result uses the original B, and so func and func2 are equal.

Obviously, this sounds like an obvious choice (1), but lets modify this scenario a bit:

import {func} from A
mock C
import {func as func2} from A

In this case, we're mocking a module that has no connection to A, yet A is still reloaded. Which is weird. Now option 2 makes more sense because in this case module variables stay the same because now func and func2 are the same.

@searls
Copy link
Member Author

searls commented Jul 15, 2022

where the user is surprised that no mocking occurs even if mocking is requested, is more problematic than the latter. We should leave it at that! Yes, at the cost of module state being difference between imports.

I agree with this. It's very likely/common that a test suite will contain numerous different mockings of a same dependency.

import {func} from A
mock C
import {func as func2} from A

In this case, we're mocking a module that has no connection to A, yet A is still reloaded. Which is weird. Now option 2 makes more sense because in this case module variables stay the same because now func and func2 are the same.

I also agree with this.

It seems like solving this with ✨ magic ✨ to figure out if a re-import is desirable is going to be fraught and surprising to just about anyone, and a strict or neverReload: true flag or something should be made available for people in the one-off cases where identity checks matter.

I'm about to leave for a weekend so I'm going to revert testdouble/quibble#72 (review) but if either of you want to try introducing such a mode to quibble (and maybe a mirror property to td.replace{Esm}, that'd be splendid

Thank you both for taking the time to explain what's going on so clearly!

@searls searls mentioned this issue Jul 15, 2022
Closed
@giltayar giltayar mentioned this issue Aug 14, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@searls @giltayar @connorjclark