function identity is different for modules that are not replaced with testdouble when testdouble is replacing other ESM modules

[travi]

Description

I've been working to migrate some projects to ESM and move from sinon to testdouble in the process. I've found this to be mostly smooth, except when a function is passed to function that was replaced/stubbed with testdouble. In those cases, the td.when definition does not match as expected, causing the test to fail since the theReturn or thenResolve does not happen as intended.

In one project that is blocked by this, you can see that i converted the project from using sinon to testdouble in this commit and all of the project verification was still passing. Afterward, I converted the project to "type"="module" and updated the project to match. However, there are several cases where the inability to match passed functions causes unit test failures.

Issue

from my investigation, it appears that the module that is not replaced by testdouble is still impacted in some way, causing it to have a different identity between the test and source files. because of this, the td.when fails to match what should be the same argument between the test and the source implementation.

It isn't entirely clear to me how the loader is working to be confident, but this seems likely to be related to https://github.com/testdouble/testdouble.js/issues/492. If that is the case and you already have the information you need, feel free to close as a duplicate. Maybe my minimal reproduction repository below can still be helpful narrowing something down further?

Environment

• [x] node -v output: v16.16.0
• [x] npm -v (or yarn --version) output: 8.11.0
• [x] npm ls testdouble (or yarn list testdouble) version:

$ npm ls testdouble
testdouble-esm-repro@ /Users/travi/development/travi-test/testdouble-esm-repro
└── [email protected]

$ npm ls quibble
testdouble-esm-repro@ /Users/travi/development/travi-test/testdouble-esm-repro
└─┬ [email protected]
  └── [email protected]

Example Repo

• [x] Create a minimal repository that reproduces the issue
• [x] Make sure that a fresh clone can run only npm it and observe the issue
• [x] Link to that repo here

https://github.com/travi-test/testdouble-esm-repro

[searls]

cc/ @giltayar -- is this possibly related to recent changes?

[giltayar]

On it! Great repro repo, @travi. Thank you.

[giltayar]

The tests pass on [email protected], but fails on [email protected]. So it is related to recent changes.

[giltayar]

Of course. 0.6.10 was the version where we "fixed" the loader (0.6.9 also fails the tests), but decided to rollback the changes.

[giltayar]

So, yes, after consideration, this is the case discussed in length here: https://github.com/testdouble/testdouble.js/issues/493#issuecomment-1185373832.

We decided on keeping the behavior of testdouble up to then: doing a reload of all modules whenever we mock something, at the price of killing identity of statics.

@searls you were on vacation then, and did not have the time to really consider the alternative. Perhaps now is the time to do so? I still stick with my choice, which is to reload and not surprise the user, at the cost of static identity. But I'm not strong on that decision. I think. ☺️🤷‍♂️

[travi]

Is there some sort of alternative in the current state? I'm not necessarily opposed to using a matcher or something along those lines in the td.when to confirm that the identities match, but it seems like that comparison should be possible somehow.

[giltayar]

@travi not sure how a comparison that is not the identity comparison would work. You could implement it yourself by attaching a "hidden" property to the function and checking that.

[travi]

not sure how a comparison that is not the identity comparison would work

Agreed. Just wasn't sure if there was something exposed that I was overlooking.

I guess just consider me hopeful that this is a use case that can be enabled in some way.

[searls]

Sorry I don't have a better answer. FWIW, one of my initial motivations in writing td.replace was the Node docs saying that a module was, in practice, always loaded once and cached for the life of the process, there was no runtime guarantee that this would be the case. As a result, I chose to design module replacement around reloading, to ensure maximum data isolation between tests

[travi]

Ok, understood. It is definitely disappointing to hear that it wont be possible to support this use case, but i think I understand the limitation.

Regardless, it sounds like this is covered in #493, so i'll close out this issue. thanks for digging in and confirming @giltayar and @searls