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

question: nodejs plugin and esm #80

Open
boneskull opened this issue May 6, 2019 · 16 comments
Open

question: nodejs plugin and esm #80

boneskull opened this issue May 6, 2019 · 16 comments

Comments

@boneskull
Copy link
Contributor

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!

@theKashey
Copy link
Owner

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).

@boneskull
Copy link
Contributor Author

thanks for your help and an explanation!

@boneskull
Copy link
Contributor Author

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

@boneskull
Copy link
Contributor Author

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

@boneskull
Copy link
Contributor Author

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

@boneskull
Copy link
Contributor Author

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

@theKashey
Copy link
Owner

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

@boneskull
Copy link
Contributor Author

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.

@theKashey
Copy link
Owner

theKashey commented May 9, 2019

Check - avajs/ava#665 and avajs/ava#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

@boneskull
Copy link
Contributor Author

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

@theKashey
Copy link
Owner

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.

@boneskull
Copy link
Contributor Author

boneskull commented May 10, 2019

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

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

@theKashey
Copy link
Owner

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

@theKashey
Copy link
Owner

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)

@boneskull
Copy link
Contributor Author

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

theKashey added a commit that referenced this issue May 16, 2019
example distilling problem found in #80
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

2 participants