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

Add test to catch issue with preserveCache not preserving correctly #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KarlPurk
Copy link

This PR adds an additional test to catch an issue with preserveCache not preserving the cache correctly. Currently this test fails as the bug has not been fixed.

This PR relates to #133

@bendrucker
Copy link
Collaborator

I went and applied your test against 1.7.0 (and a few earlier minor versions). This behavior isn't a regression—it's never worked. I'll continue poking around in require.cache and see what's happening here.

@thlorenz
Copy link
Owner

BTW I don't know that it makes sense to keep the module you are proxyquiring in the cache at all.
In order for dependencies that it requires to get swapped out it needs to be re-required and thus be removed from the cache.

Therefore your behavior never worked as you expect.
It is an antipattern to depend on side effects when being required and/or depending on them happening only once.

Bottomline is that if you proxyquire something more than once and want to register different stubs for it each time, it HAS to delete it from the cache.

@KarlPurk
Copy link
Author

KarlPurk commented Jul 1, 2016

@thlorenz I don't think this is about what's right or wrong, we could debate that all day long. The documentation states that preserveCache allows you to restore behaviour to match node's require. This isn't true.

The documentation doesn't accurately describe the true behaviour of proxyquire. One side has to change, either the docs need to be updated to describe the true behaviour or the code should be updated to implement the documented behaviour.

@thlorenz
Copy link
Owner

thlorenz commented Jul 1, 2016

Yes, @KarlPurk you are right with the preserveCache option it should actually not delete the proxyquired module from the cache, basically we need to ensure the behavior outlined in the example of the readme:

proxyquire.preserveCache();

var foo1 = proxyquire('./foo', stubs);
var foo2 = proxyquire('./foo', stubs);
var foo3 = require('./foo');

// foo1, foo2 and foo3 are the same instance
assert.equal(foo1, foo2);
assert.equal(foo1, foo3);

If it is not behaving like this currently I agree that things are broken.
Could you possibly git bisect and run these tests to narrow down what commit changed the behavior?

What I'm not entirely sure about is though is why these tests are passing and how they are different than the tests you provided.
It's been a while I looked at the code too closely, so @bendrucker may have a better answer here.

@KarlPurk
Copy link
Author

KarlPurk commented Jul 1, 2016

@thlorenz I think @bendrucker has already done this and determined that this isn't a regression because the functionality has never been implemented as described.

Looking at the test, I believe the pass here is a false possitive and the test is actually incorrect. Changing the test to the following reveals the true result:

    it('defaults to preserving the cache', function() {
      var proxyquire = require('..');
      var original = proxyquire('./samples/foo', {});
      original.state = 'cached';

      var proxyFoo = proxyquire('./samples/foo', { 'path': { } });

      var foo = proxyquire('./samples/foo', {});
      assert.equal(foo.state, 'cached');
      assert.equal(foo, original);
    });

@KarlPurk
Copy link
Author

KarlPurk commented Jul 1, 2016

I've fixed the test in my fork: KarlPurk@886615c

Would you like me to create a PR to fix the broken test and close this PR?

@thlorenz
Copy link
Owner

thlorenz commented Jul 1, 2016

No the test was fine .. we don't want to change tests that exist.
My question was more along the lines what is different in your test and then what is the correct behavior here, should your test pass or not and/or are the docs misleading, specifically does the example test I posted above pass since its part of the docs and should?

@KarlPurk
Copy link
Author

KarlPurk commented Jul 1, 2016

Okay, so to answer your question the main difference between the tests is the tests I supplied use proxyquire exclusively to test the behaviour whereas the test you linked to uses a mix of proxyquire and require.

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

Successfully merging this pull request may close these issues.

3 participants