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

Don't replace globally #39

Closed
umaar opened this issue Nov 12, 2020 · 9 comments
Closed

Don't replace globally #39

umaar opened this issue Nov 12, 2020 · 9 comments

Comments

@umaar
Copy link

umaar commented Nov 12, 2020

Hey! This project is great and minimal, nice work!

I noticed it doesn't support mocking a dep, without affecting global use of such a module. E.g. my app source uses node-fetch which I want to mock out for an integration test. But interestingly my test code also uses node-fetch, but quibble doesn't seem to support this use case. Any workarounds? .reset() doesn't help here.

@searls
Copy link
Member

searls commented Nov 12, 2020

As a matter of good TDD / mocking practice, even though quibble can fake out 3rd party modules it doesn't mean you necessarily should. In general, my approach is to write custom little adapters to third party dependencies so I have some scar tissue indirection with an API that I control such that I can make it conform to the style/shape of the rest of my app's code. (You can take or leave this design and testing advice but [here's a talk about mocking fwiw](Write a test-scoped wrapper of https://blog.testdouble.com/talks/2018-03-06-please-dont-mock-me/))

The reason I mention this is you could always have a module like lib/wrap/fetch that depends on node-fetch and exports the functions you need. Then in your test just td.replace() that lib/wrap/fetch dep, so that your test-scoped code can continue to load node-fetch.

If you don't want to do that, then you can always have your test-scoped support code require node-fetch in advance of any td.replace calls and cache it to some variable and reference that

@searls searls closed this as completed Nov 12, 2020
@umaar
Copy link
Author

umaar commented Nov 12, 2020

Cool I'll watch that video thanks. A few points:

  • This is an integration test, and mocking out node-fetch feels sensible (because it calls a third party API outside my control)
  • While yes, adapters for like stripe or some other module makes sense normally, this is node-fetch - something meant to replicate the Fetch API. If ever its API design changes, I'd want to change my app source code to match, so a middle layer doesn't make sense in this case.
  • The thing I'm making is to show best practices, and personally I think changing application source code to make tests easier to build is an anti-pattern.

In light of all that:

  • Not sure about td.replace, I'm just using quibble for its excellent ESM support! I have tried many variations of importing node-fetch before/after, with/without reset(), with/without dynamic imports, but ultimately whenever I log what's happening in both application source code & test code (e.g. with console.log(await import('node-fetch')) from test code) either they both get mocked, or neither gets mocked, whereas what I want is the application source code to use the mocked version, and the test code to use the real version. Maybe something weird happening with node.js internals? Might explore something like nock, but really hope quibble can support this at some point/or node.js fixes whatever strange module caching behaviour might be happening.

Thanks for explaining your thoughts and sharing that vid!

@searls
Copy link
Member

searls commented Nov 12, 2020

Sorry, I only ever use quibble via td.js so I should have expressed it in terms of quibble's API. For ESM specific things I need to rely on @giltayar to chime in

@giltayar
Copy link
Contributor

@umaar not sure I understand what the problem you have is: if I understand correctly, while running your app code in a test, that app code uses node-fetch, which you want to mock, but your test code itself also uses node-fetch, but you don't want to mock that?

If my description of the problem is correct, then there's not a lot I can do: between the time you mock a module usingquibble(...) and the time you call reset, that module is mocked globally. This is true when mocking ESM and when mocking CJS (right, @searls)?

Wait, maybe you can use quibble.ignoreCallsFromThisFile? While this won't give you exactly what you need (it doesn't look at the call stack, just at the immediate module that called the mocked module), it may be good enough for your purpose.

@umaar
Copy link
Author

umaar commented Nov 14, 2020

Hey your understanding is correct @giltayar

But anyway it seems one of the big issues is this

ES modules are resolved and cached based upon URL semantics.

So importing is cached! That would explain the problems I've been having, and why reset() doesn't appear to be doing anything in my project. Don't suppose you know of any workarounds? Related issue: nodejs/node#49442

Oh and here's an example file which demonstrates the problem: https://github.com/umaar/postcode-checker/blob/85e4e57b1aa9b29316812e7eca72398b916a9559/test/unit/test.js - there you'll notice await quibble('node-fetch', mocked); is called twice with different mock functions, but it seems the latter mocked function affects all instances, with reset() having no effect.

One workaround I'm thinking is to mock node-fetch one time only, and within the mock function body, use the switch statement to return different responses based on the input - for this use case I guess that'd be ok. Not ideal, but not sure what else.

Update: Searching around, I happened to stumble across this deno related comment by @kitsonk:

It [quibble] basically creates a unique specifier for every time the module is requested to get around the challenge of ES modules being static once resolved.

ha so maybe I've misunderstood and quibble does indeed attempt to fix this already! But then it wouldn't explain why mocking node-fetch with different variations doesn't seem to work as expected. Using node.js v15 fwiw.

@giltayar
Copy link
Contributor

@umaar as you surmised, caching is not the problem. I use various magic tricks in the quibble ESM loader to deal with his (read all about it: https://dev.to/giltayar/mock-all-you-want-supporting-es-modules-in-the-testdouble-js-mocking-library-3gh1).

Did you read my previous comment, and try out quibble.ignoreCallsFromThisFile?

@umaar
Copy link
Author

umaar commented Nov 15, 2020

@giltayar sorry I didn't see any docs for ignoreCallsFromThisFile and wasn't sure about it. But that's a really interesting blog post I will read it in more detail!

Here's the issue I'm seeing:

 node --loader=quibble ./node_modules/.bin/ava test.js
import quibble from 'quibble';
import test from 'ava';

test('test 1', async t => {
	await quibble.esm('node-fetch', null, 'mock v1');
	const {default: fn} = await import('fn.js');
	quibble.reset();
});

test('test 2', async t => {
	await quibble.esm('node-fetch', null, 'mock v2');
	const {default: fn} = await import('fn.js');
	quibble.reset();
});

test('test 3', async t => {
	await quibble.esm('node-fetch', null, 'mock v3');
	const {default: fn} = await import('fn.js');
	quibble.reset();
});

I put a console.log in the fn.js file.

The issue is: it's only ever the last quibble mock that seems to take effect. If I log the value of node-fetch from fn.js, it's always the value mock v3 or whatever the last mock was. In fact, I don't see any console.log's for the first two tests, only for the very last one. So the terminal output is: the value of fetch is mock v3 and that's it.

It sounds like you've already dealt with caching well. So now I'm thinking maybe it's something about how AVA, the test runner I'm using, runs tests. Maybe something to do with running tests in parallel. Whatever it is, putting only 1 test per file helps me avoid this problem.

@giltayar
Copy link
Contributor

@umaar I'll try ava and see what i can figure out. I'll also try the same in Mocha to see whether it's specifically Ava.

@umaar
Copy link
Author

umaar commented Nov 17, 2020

Thanks, I guess ava runs tests concurrently so maybe that has an effect.

Also heads up, I tried running the tests for that repo in windows, and I'm seeing some funny behaviour - quibble doesn't seem to be working as expected. Haven't looked in detail though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants