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

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

Closed
6 tasks done
travi opened this issue Aug 12, 2022 · 10 comments
Closed
6 tasks done

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

travi opened this issue Aug 12, 2022 · 10 comments

Comments

@travi
Copy link

travi commented Aug 12, 2022 •
edited

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 #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

  • node -v output: v16.16.0
  • npm -v (or yarn --version) output: 8.11.0
  • npm ls testdouble (or yarn list testdouble) version:
$ npm ls testdouble
testdouble-esm-repro@ /Users/travi/development/travi-test/testdouble-esm-repro
└── testdouble@3.16.6

$ npm ls quibble
testdouble-esm-repro@ /Users/travi/development/travi-test/testdouble-esm-repro
└─┬ testdouble@3.16.6
  └── quibble@0.6.14

Example Repo

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

https://github.com/travi-test/testdouble-esm-repro
@searls
Copy link
Member

searls commented Aug 13, 2022

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

@giltayar
Copy link
Collaborator

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

@giltayar
Copy link
Collaborator

The tests pass on quibble@0.6.10, but fails on quibble@0.6.11. So it is related to recent changes.

@giltayar
Copy link
Collaborator

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
Copy link
Collaborator

So, yes, after consideration, this is the case discussed in length here: #493 (comment).

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
Copy link
Author

travi commented Aug 14, 2022

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
Copy link
Collaborator

@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
Copy link
Author

travi commented Aug 14, 2022

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
Copy link
Member

searls commented Aug 14, 2022

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
Copy link
Author

travi commented Aug 15, 2022

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

@travi travi closed this as not planned Won't fix, can't repro, duplicate, stale Aug 15, 2022
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 @travi @giltayar