thlorenz / proxyquire

🔮 Proxies nodejs require in order to allow overriding dependencies during testing.
MIT License
2.75k stars 100 forks source link

Subobjects in proxyquired modules are not updating on re-proxyquing #102

Closed Alexsey closed 8 years ago

Alexsey commented 8 years ago

This project contains working demonstration of the bug. Setup of a problem is

  • we want to test module foo that requires module bar that contains object that contains function g. We need to stub g
  • proxyquiring foo and stab bar with empty object fake
  • override g on fake (g is not directly on bar, so in is like fake.f.g = ...)
  • re-proxyquiring foo while stubbing bar with new empty object
  • problem occurs - g is still overridden

For comparison in the root of bar there is also function h (not wrapped with f object) and it acts as expected

noPreserveCache is active. So any changes on modules should be dropped on every new proxyquire. But it is not

UPD: one of the possible solutions is to clean require.cache of bar before re-proxyquiring foo (like delete require.cache[require.resolve('./bar')]). Not the thing I really want to do while writing simple tests...

UPD: this solution is even worse than it looks: if required foo require bar which requires baz then you need to clean cache for baz. And you Definitely don't want to know about baz while you are testing foo

May be I'm missing something and there is a better way to restore modules state between tests? For now it looks like much easier to stab all methods with sinon's sandbox

kilianc commented 8 years ago

Not sure if I have the same problem but something similar is happening to me.

Consider the following code:

const auth = proxyquire('./lib/middleware/auth', {
  'lib/model': {
    AuthTokens: {
      refresh: (token) => (done) => done(null, token === 'auth-token')
    }
  }
})()

AuthTokens.refresh is permanently mocked even when require('./lib/model').AuthTokens.refresh, am I missing something or this is not how it's supposed to work?

Alexsey commented 8 years ago

Have you tried to use noPreserveCache?

bendrucker commented 8 years ago

You definitely need noPreserveCache, we can reopen if that's not it

kilianc commented 8 years ago

@bendrucker it's the other way around, when I call require it returns the proxyquired mocked version. Not sure if my issue is separated.

bendrucker commented 8 years ago

Same thing. If you don't tell proxyquire not to use the require cache it will.

Alexsey commented 8 years ago

@bendrucker Look at example in initial message. There is noPreserveCace but it works bad

Alexsey commented 8 years ago

And it behave differently for function that is directly in export object, and with one that is not directly on in

kilianc commented 8 years ago

@bendrucker ok, but I would consider changing this behavior anyway, I can't think of any case where you want proxyquire to globally mock a library and affect require, to me that's the whole point and reason why I am using it. Shouldn't it be the other way around?

If I am wrong I would personally rather export all the dependencies I have to mock in the test suite and have more control.

You may want to consider a .restore() function if the reason this is behaving like this is purely a technicality.

Alexsey commented 8 years ago

@bendrucker could you please look at the initial problem and answer why this it not a bug, or reopen this issue?

There is a link at the start of initial post on another project on GitHub with code that represents problem

Questions are

  1. Why noPreserveCace do nothing?
  2. Why g and h behave differently?
bendrucker commented 8 years ago

Don't have time to look into it this week, will reopen in case anyone else wants to contribute

kwhitaker commented 8 years ago

I may have a related case to this, or it might be something that I'm misunderstanding about proxyquire.

In my case, I'm testing some methods normally, but then I need to stub those methods later.

import * as methods from './some-module'
import proxyquire from 'proxyquire'
proxyquire.noPreserveCache()

... do some tests on `methods`
... later
const mock = proxyquire('./some-module', {
  someMethod: sinon.spy()
})

However, mock ends up being equal to methods, and therefore I can't spy on the internals.

cbshakumar commented 8 years ago

I think this may be related. I have been trying to understand if there was a way around this behavior.

var proxyquire = require('proxyquire').noPreserveCache();
var expect = require('expect.js');
var TestModule = '../index.js';

describe('Test', function(){

  var dependencyMock;
  var test;
  before(function(){
    dependencyMock = function(){
      this.doStuff = function(callback){
        return callback(null, 'blah');
      };
    };
    var mocks = { './dependency.js': dependencyMock };
    var Test = proxyquire(TestModule, mocks);
    test = new Test();
  });

  it('does things', function(done){
    test.doSomething(function(err, data){
      expect(data).to.be('blah');
      done();
    });
  });

  describe('dependency returns banana', function(){
    before(function(){
      dependencyMock = function(){
        this.doStuff = function(callback){
          return callback(null, 'banana');
        };
      };

    });

    it('does banana things', function(done){
      test.doSomething(function(err, data){
        expect(data).to.be('banana');
        done();
      });
    });
  });
});

The test for banana fails because it still comes back with the original mocked data of 'blah'.

Alexsey commented 8 years ago

@cbshakumar Just before 'does banana things' test runs both of your before functions would be executed (Mocha works that way) so doStuff would be reassigned twice. Probably Mocha execute firstly inner bofore and than outer before that is why in your test it returns 'blah' in stead of 'banana'. Log/debug your code to check the order of befores execution

Alexsey commented 8 years ago

@kwhitaker you are trying to pass as second parameter of proxyquire call object with key that is the name of the method. proxyquire doesn't work that way: keys should be the names of packages that require module in first argument and values should be object with keys that are names of methods in required packages. Look at this section of the docs

cbshakumar commented 8 years ago

@Alexsey screen shot 2016-05-04 at 4 18 04 pm

before is run only once for each describe block it is in. This is not the beforeEach function from mocha. But you can see from the repl/debug that it has already ran through the outer before statement by the time it has gotten to the inner one.

kwhitaker commented 8 years ago

@Alexsey thanks for clearing that up. I think I might need something like https://www.npmjs.com/package/rewire for this case.

Alexsey commented 8 years ago

@cbshakumar Yes, sorry, probably I confused them. Any way your problem is definitely in fact that you are reassigning your local dependencyMock variable with a new function while proxyquire continues to work with an old one. There is now way in JavaScript proxyquire can detect change of a target of a link

cbshakumar commented 8 years ago

@Alexsey right, I would imagine that just modifying the object instance itself, proxyquire should pick this up since it has the object reference still and I have no preserve cache set. How else can I get proxyquire to automatically pickup the modified object reference?

Alexsey commented 8 years ago

@cbshakumar proxyquire itself can't help you. And there is no way in JS to send a pointer. But there are some tricks you can do. First I can imagine is to change mocks to

    var mocks = { './dependency.js': function () {
      return dependencyMock.apply(this, arguments)
    }};
cbshakumar commented 8 years ago

I was able to get tests passing by changing them to use an internal instance for the mock.

var proxyquire = require('proxyquire').noPreserveCache();
var expect = require('expect.js');
var TestModule = '../index.js';

describe('Test', function(){

  var dependencyMock;
  var instance;
  var test;
  before(function(){
    console.log('outer');
    dependencyMock = function(){
      this.doStuff = function(callback){
        instance.doStuff(callback);
      };
    };
    instance = {
      doStuff: function(callback){
        return callback(null, 'blah');
      }
    };
    var mocks = { './dependency.js': dependencyMock };
    var Test = proxyquire(TestModule, mocks);
    test = new Test();
  });

  it('does things', function(done){
    test.doSomething(function(err, data){
      expect(data).to.be('blah');
      done();
    });
  });

  describe('dependency returns banana', function(){
    before(function(){
      instance.doStuff = function(callback){
        callback(null, 'banana');
      }
    });

    it('does banana things', function(done){
      test.doSomething(function(err, data){
        expect(data).to.be('banana');
        done();
      });
    });
  });
});

Maybe we should have some kind of abstraction/helper code to remove the cruft of having to do this.

Alexsey commented 8 years ago

Yep, that is a similar way. I don't see how such helper could be a part of proxyquire... but if you do - you should probably create a separate issue with suggestion as fare as we figure out that it is not about the bug from the initial post

cbshakumar commented 8 years ago

Sure

bendrucker commented 8 years ago

Thank you for the diligent effort in reproducing this @Alexsey

I just dug in for a bit and was able to identify the issue. The stubs (child of a child) were still stuck in the require cache even though the child itself was deleted. I've made an addition to delete those as well. I've also incorporated a virtual identical unit test to your repro project and verified that the changes pass your tests.

Alexsey commented 8 years ago

This is great! Thank you for fixing it =)