rewiremock icon indicating copy to clipboard operation
rewiremock copied to clipboard

question: nodejs plugin and esm

Open boneskull opened this issue 6 years ago • 16 comments

Hi,

I was having some trouble using the nodejs plugin and relative paths with esm and rewiremock.module(). I'm unable to provide a minimal case to reproduce the problem I'm having, but I'll try to explain:

  • lib/foo.js is a module
  • lib/baz.js is another module
  • lib/bar/quux.js is yet another module
  • lib/bar/quux.js depends on lib/foo.js
  • lib/foo.js depends on lib/baz.js
  • test/bar/quux.spec.js is a test for lib/bar/quux.js, which uses rewiremock to stub lib/foo.js
  • test/foo.spec.js is a test for lib/foo.js, which uses rewiremock to stub lib/baz.js

This is how I'm using rewiremock:

  • stubs uses module paths relative to the test files
  • I'm using overrideEntryPoint() in a separate test helper file to load rewiremock
  • I'm running my tests via mocha -r esm, and with the nodejs plugin enabled.
  • I'm using the rewiremock.module(() => import('/path/to/module') syntax.

If I run each of those two tests individually, they both pass. If I run them together, test/bar/quux.spec.js runs first, then test/foo.spec.js runs. The latter fails, as it can't find lib/baz.js, which is stubbed via the relative path ../lib/baz.

In the resulting stack trace, test/bar/quux.spec.js appears at the top. Running the debugger, when test/foo.spec.js is trying to stub ../lib/baz, the "parent" module is test/bar/quux.spec.js, which seems incorrect.

A couple things I've noticed:

  • If I disable the nodejs plugin and use paths relative to the files under test (e.g., lib/foo.js stubs ./baz instead of ../lib/baz), all tests pass together (in other words, I'm not blocking on a fix)
  • Using rewiremock.enable() paired with rewiremock.disable() seems to have no effect whatsoever

So my question is if I should be using the nodejs plugin, or a different plugin, and how can I specify relative-to-test-file module paths to be stubbed? Is there a bug here?

Thanks for your work on this module!

boneskull avatar May 06 '19 21:05 boneskull

A file could be mocked only if it was not required before. Then it could be replaced on require. To achieve this rewiremock wipes all required files on .enable(on any command containing this command inside, like .module).

This operation is very slow, you have to traverse ALL files and determine - should you wipe them from a cache, or not. Other libraries are wiping all the cache, but that's even slower, and leads to many issues with singletons.

Long story short - but in your case, it's able to understand which file should be wiped. So it's not wiped, and, as a result, not replaced.

Why it working in a first run - algorithms to determine which file should be wiped, and which should be replaced are different. It's easy to understand which to replace - you have a "parent" and name, relative to a parent, is easy to handle. During cache wipe it's the only name, so... shit happens.

Name resolution is hard stuff, and even if rewiremock was created to solve it, it did not, and I recon - should not. It's not the fight we might win.

How to fix

  1. Add usedByDefault plugin. It would make clear that stub you tried to use was not used.
import { addPlugin, plugins } from 'rewiremock';
addPlugin(plugins.usedByDefault);
  1. I would propose to use guided mocks, as they were named in a readme:
const module = rewiremock.proxy( () => require('./path/to/module'), () => {
  rewiremock(() => require('/path/to/otherModule').with({});
});

In this case it's impossible to make a mistake - name resolution is handled by nodejs itself, and you will also maintain type information, if you have one. But - you have to use names relative to the test file.

  1. There is not properly typed and documented command - rewiremock.forceCacheClear.
forceCacheClear(false) // normal cache clearing, default
forceCacheClear(true)  // aggressive cache clearing. Will restore module system like it was before, like you never execute any module via rewiremock

rewiremock.forceCacheClear(true) should solve your problem.

PS: You dont have to use import and async APIs unless you use .mjs. And rewiremock would not work with .mjs by design (of ESM modules, which are not controllable in a old way).

theKashey avatar May 06 '19 22:05 theKashey

thanks for your help and an explanation!

boneskull avatar May 06 '19 22:05 boneskull

Looks like forceCacheClear(true) is a setting and needn't be called before every test.

boneskull avatar May 08 '19 23:05 boneskull

I think I'm dumb and should just be able to stub it with Sinon

boneskull avatar May 08 '19 23:05 boneskull

deleted a question above, as I did in fact not need rewiremock at all for what I was doing.

boneskull avatar May 09 '19 00:05 boneskull

It's worth mentioning that I didn't end up needing forceCacheClear(true) either. just proxy works fine w/ guided mocks.

boneskull avatar May 09 '19 00:05 boneskull

Sinon itself a recommendation not to use sinon for dependency mocking, but its not quite clear. Take a look at this PR.

theKashey avatar May 09 '19 00:05 theKashey

hm. OK, so I’m unsure why rewiremock doesn’t work with fs for me using the pattern above.

I can try to come up with a more complete example (the one I deleted was a bit contrived).

regarding the PR, I’m reading through more of it and your linked articles (and a video linked from the articles), but why I shouldn’t just stub fs.readdir is not immediately clear to me.

boneskull avatar May 09 '19 02:05 boneskull

Check - https://github.com/avajs/ava/issues/665 and https://github.com/avajs/ava/issues/1359 They are both about the same - mocking fs for tests, while fs is still required for ava itself.

Never modify globals.

That's something from the testing basics - you shall test subject under test and not anything else. Changing fs is changing it for almost all consumers, ie excluding the ones with const readdir = fs.readdir. That's poisonous and undefined behavior.

I've tried to explain something about it here - https://dev.to/thekashey/please-stop-playing-with-proxyquire-11j4

theKashey avatar May 09 '19 03:05 theKashey

This is a good point, and one I often never take into consideration given I know the implementation details of my testing framework. In other words, I know what Mocha touches and what it doesn't--fs.readdir isn't one of those things.

That talk you linked (from searls) is very informative. Given I'm finding mocking this particular bit of my code to be difficult, I'm going to take another pass at the code's structure. Likewise, I'm going to stop using mockThroughByDefault, which seems like a footgun...

boneskull avatar May 09 '19 04:05 boneskull

In the same time mockThrough is a default behavior for jest.mock - ie provide an interface-compatible version of a module, which does, however, does not do anything.

theKashey avatar May 09 '19 04:05 theKashey

sorry to bother again. I've been trying to figure out how to get around this for the past two workdays, and have been trying to follow your suggestions.

I have a test which uses rewiremock.proxy() to load module A, and mock module B:

    subject = rewiremock.proxy(
      () => require('../A'),
      () => {
        rewiremock(() => require('../B'))
          .with({foo: 'bar'})
      }
    );

But I also have a test which I want to run against B. B requires module C, and I want to stub that out:

    subject = rewiremock.proxy(
      () => require('../B'),
      () => {
        rewiremock(() => require('../C'))
          .with({bar: 'baz'})
      }
    );

Running these tests together (not individually), the usedByDefault plugin which I've enabled throws an error. It claims that module C is not being used by module B. This is seemingly untrue, but...

If I disable the usedByDefault plugin, I can see that the original B runs, but C is not mocked. It does not replace prop bar with baz, in other words.

An initial call in my harness code to rewiremock.forceCacheClear() or rewiremock.forceCacheClear(true) does not affect the result:

import rewiremock, {addPlugin, overrideEntryPoint, plugins} from 'rewiremock';

import sinon from 'sinon';

// see https://github.com/unexpectedjs/unexpected/issues/631
const expect = require('unexpected');

global.sinon = sinon;

global.expect = expect
  .clone()
  .use(require('unexpected-sinon'))
  .use(require('./unexpected-rxjs'));

overrideEntryPoint(module);

addPlugin(plugins.usedByDefault);

rewiremock.forceCacheClear(true); // or anything else
global.rewiremock = rewiremock;

(I prefer having stuff like this in global scope for convenience; if rewiremock is incompatible with this sort of use, please advise!)

boneskull avatar May 10 '19 22:05 boneskull

(to be clear, that first test against A runs before the test against B)

boneskull avatar May 10 '19 22:05 boneskull

It seems that tests are not properly isolated from each other. Let me try to create a test for it.

theKashey avatar May 11 '19 00:05 theKashey

The tests is passing. Something else is not working as expected. https://github.com/theKashey/rewiremock/blob/master/_tests/issues/80/test.spec.js

Possible problem - one of the files is already mocked, and nested mocks are poisoning module (stored in a "scope") settings. Theoretically it's possible. If you have a mock not nested in a "scoped" function (proxy, module, around, inScope)

theKashey avatar May 11 '19 01:05 theKashey

Thanks, I’ll use your test to try to reproduce

boneskull avatar May 11 '19 03:05 boneskull