Destroy all caches to work with angular-mocks tests

[henrahmagix]

angular-cache should clean itself to work with angular unit tests. angular-mocks calls $rootScope.$destroy in its cleanup phase for every spec, so angular-cache can clean itself in an event listener on the $destroy event.

Without this, angular-cache errors when more than one spec injects the CacheFactory and the app code is creating caches with it.

For #236

@jmdobry Please merge, ta! I wasn't sure about only destroying if angular-mocks is present, so I thought that this Angular library should always clean itself when Angular resets, which is what $rootScope.$destroy is, I think.

If you know of any other way to detect when Angular resets (that would coincide with how angular-mocks works), I'll be happy to change this =)

[codecov-io]

Current coverage is 86.15% (diff: 100%)

No coverage report found for master at b491ed8.

Powered by Codecov. Last update b491ed8...b4f8f8e

[jmdobry]

I'm leery of adding something to this library that's meant to run in a test environment.

What prevents you from calling destroyAll in the afterEach clause in your tests?

[henrahmagix]

I'm leery of adding something the this library that's meant to run in a test environment.

I understand. However, since this is an Angular library, I think it should be lenient with angular-mocks where currently it errors.

What prevents you from calling destroyAll in the afterEach clause in your tests?

Nothing, though it is annoying =) Especially if I have to add it to anything that test that injects something that depends on something else that uses this. I'll end up mocking it and copy-pasting lots of code. I just think this library can do a little bit to make it much simpler for anyone using this in their unit tests.

If they swap out $cacheFactory for this (which is what I did), suddenly the unit tests will fail, and it's not immediately apparent why.

Although you could solve that with readme documentation, I think the better solution is to apply an Angular method of memory management, where the caches is an Angular factory/service, so it can be created/destroyed in the correct way. Or at least make it act in that way, which is what this pull-request does.

[henrahmagix]

I've been looking at the injector in angular and angular-mocks, because I hope there would be something in there, or something based around Angular's bootstrap phase (if angular-mocks uses that). That would be a nicer way of doing it: when Angular bootstraps itself, clear the caches to make sure they're gone.

[Jiia]

What prevents you from calling destroyAll in the afterEach clause in your tests?

Since you can inject only once per module registration I'd have to copy and paste the injection and destroying of CacheFactory to the 100+ registrations I currently have. If every other library handled test environments this bad it would be impossible to write tests at all.

I'm leery of adding something the this library that's meant to run in a test environment.

Angular itself has code that's only there to run in tests. Please don't ruin your otherwise great library by being overly paranoid.

[deini]

If every other library handled test environments this bad it would be impossible to write tests at all.

Please don't ruin your otherwise great library

Wow, whats up with you guys? Chill.

Why don't you guys just mock the whole library? Why are you guys even using it in your tests?

[jmdobry]

I've looked through angular.js and angular-mocks.js, and I can't find anything to indicate that Angular's own $cacheFactory calls destroy on its own caches anywhere. This is because the $cacheFactory provider creates a new cache factory from scratch every time the provider is invoked.

With angular-cache however, it's just a wrapper around CacheFactory, which doesn't currently provide any facility for creating a new cache factory.

The correct solution to this problem is to modify CacheFactory to support the instantiation of a new CacheFactory, which angular-cache can then use to make a new instance when the angular-cache provider is invoked.

This will probably require some breaking changes to CacheFactory, while angular-cache itself will only need a small backwards-compatible change.

[henrahmagix]

Why don't you guys just mock the whole library? Why are you guys even using it in your tests?

@deini The problem, as I now realise I didn't explain very well in the description (apologies for the confusing description), is that more than one spec for anything that uses angular-cache will always throw an error due to how angular-cache wraps CacheFactory, as @jmdobry has explained. I mistakenly described using it in my tests as the problem: that wasn't the issue.

It is possible to mock it for every spec, but as @Jiia points out, that can be tedious for anything more than a couple of tests. Even if you do that, that doesn't allow integration tests, which are useful for testing library upgrades.

The correct solution to this problem is to modify CacheFactory to support the instantiation of a new CacheFactory

@jmdobry You're right, that is the correct solution =) In my timidity, I thought it better to make the smallest, simplest change possible, without involving the CacheFactory library. I'd love to help out with making that change if you'd like help, otherwise I'll leave it to you.

[jmdobry]

I'll take a look at it on the plane tomorrow.

[henrahmagix]

Thanks for implementing the correct solution @jmdobry!

[jmdobry]

Took 10 hours over several evenings, but I still haven't released a new angular-cache because there are a few issues with the angular-cache tests to resolve.