fix(injector): allow multiple loading of function modules

[shahata]

move the loadedModules lookup to be performed only on strings in order to allow loading functions that look the same. this issue caused only the first angular.mock.module(object) invocation to work since the loadModules lookup blocked all following invocations.

closes #7254

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#7255)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[shahata]

Trying to bump this :)

The current behavior of angular.mock.module(object) is really unexpected because of this and I've already seen many people bump into this issue and spend an hour trying to figure out why their test don't work.

The current situation is that the following test does not pass since the second call to angular.mock.module(object) is ignored. This is very confusing for anyone experiencing this:

angular.module('myApp', []).value('val1', {some: 'value'}).value('val2', {other: 'value'});

describe('test my thing', function () {
  beforeEach(function () {
    module('myApp');
    module({val1: {some: 'mock'}});
  });

  it('should work', function () {
    module({val2: {other: 'mock'}});
    inject(function (val2) {
      expect(val2).toEqual({other: 'mock'});
    });
  });
});

Plunker - http://plnkr.co/edit/hgk3f8yiAvnXlwwjVbCJ?p=preview

[caitp]

this test description doesn't really describe what is being tested

[caitp]

also, try to avoid "should NOT", say what it DOES

[caitp]

I kind of think the appropriate fix for this is to make sure that the hashmap in apis.js understands functions as well as objects

[caitp]

Yeah, test passes if you just change

  if (objType == 'object' && obj !== null) {

to

  if (objType === 'function' || (objType == 'object' && obj !== null)) {

in src/apis.js

[caitp]

oops, didn't mean to close that

[caitp]

In my view, this can be checked in if those changes are implemented, so I'll triage it for the next beta

[shahata]

@caitp - thanks for looking into this. The problem with making this change in HashMap is that my new test passes, but ~400 other tests fail :)

I looked into the reason the other tests fail, and I think the problem is limited to angular's test suite. However, solving it is very problematic. I'll try to explain. (sorry, but it is going to be long :D)

The reason all those tests fail is that testabilityPatch.js sets uid = ['0', '0', '0']; before each test. This means, for example, that nextUid() generates $$hashKey = '001' for the first module function we load in each test (in case it doesn't already have a $$hashKey).

Now, if we look at the following tests:

describe('test my thing', function () {
  beforeEach(module(function problemFn($provide) {
    $provide.value('val1', {some: 'mock'});
  }));

  it('should work', function () {
    module({val2: {other: 'mock'}});
    inject(function (val2) {
      expect(val2).toEqual({other: 'mock'});
    });
  });

  it('should work again', function () {
    module({val2: {other: 'mock'}});
    inject(function (val2) {
      expect(val2).toEqual({other: 'mock'});
    });
  });
});

Notice how problemFn is the same function instance for both tests. That means that when the first test runs, problemFn gets $$hashKey = '001' and the val2 mock gets $$hashKey = '002', but when the second test runs, problemFn already has $$hashKey = '001' from the previous test and val2 mock also gets $$hashKey = '001' (since testabilityPatch.js sets uid = ['0', '0', '0']; before each test). So in the second test, the mock will not be loaded since function:001 is already in the HashMap.

This can be solved by doing this instead:

beforeEach(function () {
  module(function problemFn($provide) {
    $provide.value('val1', {some: 'mock'});
  });
});

But it means that the beforeEach(module(function ($provide) {...})); will mostly no longer work inside angular's test suite, so I think it is not an option.

I hope I succeeded in explaining this :) Anyway, question is if you still prefer this solution over the current one. WDYT?

[caitp]

You are typically creating a new injector for each test suite, so resetting the UUID count doesn't really matter a whole lot

[caitp]

What basically ends up happening originally is this:

The hashKey function checks if an item is a non-null object. If it is, it adds the $$hashKey property with the value nextUuid() to ensure uniqueness.

For non-objects, we just treat the object itself as a key.

So what happens when you add a function as a property key? This!

function test() {}

var obj = {};

obj[test] = "Hello, world!";

console.log(obj["function test() {}"]); // Hello, world!

Because the %ToString%'d value ends up as the key. Uuid never gets involved.

(This is a multi-browser thing, not just V8. If you want to try this in FF, you can paste this into the terminal:

(function() { function test() {}

var obj = {};

obj[test] = "Hello, world!";

return obj["function test() {}"]; })()

)

So what we do to fix this is make sure that functions get the $$hashKey added too, so that we don't end up trying to add a duplicate key for the same function.

[caitp]

Oh and by the way, just so it's clear, I understand what is happening in your test failure example (I am not an idiot even though I do act like a goof quite often!), I just don't think the fix in this PR is the way to go. The right thing to do is make the hashKey api behave correctly for functions, IMO.

[shahata]

Yes, I understand all of this and I agree the solution to set $$hashKey for functions is better. The problem as I described above is that doing it the way you suggested will break ~400 unit tests. :)

Since the function instance can be reused between tests (see example above), it is possible that a function already has a $$hashKey when the test starts. This fact along with the fact that UUID gets reset every test causes a lot of trouble for those ~400 tests...

[caitp]

It breaks absolutely 0 tests in angular core, which tests are you saying it breaks? I tested this while reviewing

[caitp]

I'll send you a PR so you can run it with my alterations

[shahata]

oh, sorry. already posted a pr to show that tests are failing :)

[caitp]

Okay, so there are some failures that aren't completely addressed by this, but not very many.

It's mostly related to the expectation that Scopes will have a specific UUID, with rootScope always being 002-ish. There's some additional failures with ngRepeat's dupe minerr tests for trackBy, but these are really minor things that should be fixable in a different way.

[caitp]

My solution was to add an after() block for mocha and jasmine in angular mocks to clean up loaded modules, which fixes most of the errors. But you still end up with those unfortunate issues for $rootScope and ngRepeat.

I think a good way around this in particular would be to use a different uuid object for modules, that would be the simplest fix probably. @IgorMinar do you have a preference regarding that? --- On the other hand, this particular set of failures should never affect applications, because they can't reset the uuid object anyways

[caitp]

Anyways, this implementation is the simplest, but I don't think it's really "correct", since it allows the same function to be loaded twice --- that's not right. It's a bit more code to fix this (caitp/angular.js@angular:master...caitp:issue-7255 for one approach) , but it more correctly handles the loading of non-string modules, by preventing them from being loaded multiple times --- and, using some hacks, it avoids breaking tests

[shahata]

@caitp First of all I'm happy to see I wasn't hallucinating the test failures and the debugging session that followed :) Regarding your proposed approach to work around this - I think it is absolutely valid, and if indeed this is the way you want to do it I can wrap this up quickly (obviously some tests are missing)

The thing is, I feel it is a bit of an overkill and as you stated a bit hackish... In my opinion the current solution in this PR is indeed the simplest, as you said. I think that I don't have a problem with loading the same function twice - the way I see it, the purpose of the loadedModules HashMap in injector.js is to prevent an infinite loop of loading module requires (when module 'A' requires module 'B' and module 'B' requires module 'A'), while function modules (mainly used in tests) are simply invoked and do not have requires.

Let's see an example where someone might have a problem with double loading of a function module. Do you think that anyone might actually do this?

function moduleFn() {
  console.log('should only print once');
}
angular.module('myApp', [moduleFn, 'helperModule']);
angular.module('helperModule', [moduleFn]);

WDYT? Do you still prefer to handle multiple loading of functions correctly? Maybe we should get another eye on this?

[caitp]

It's a tiny bit simpler, but I don't think it's the right thing to do. But one of the other people can say what they'd prefer to do. Simple is good, but I am not big on only guarding against duplicate calls for string arguments

[IgorMinar]

The problem here is that we are leaking state (uuids) from one test to another. If that's fixed then the symptom should go away.

Wouldn't cleaning up all the module fns (like @caitp did here: caitp/angular.js@angular:master...caitp:issue-7255#diff-2a255ed5e9564e25ce6eb711b604f40fR2171) do the trick?

[caitp]

@IgorMinar cleaning up all the module functions/objects "kind of" works, but it causes problems for internal tests which are expecting rootScope to always have a specific ID (for example), and some other failures in tests for track by expressions in ngRepeat, things like that. It's not a lot of failures, fewer than 30. I don't think it would affect 3rd party applications though

[IgorMinar]

sounds like the tests are wrong and need to be fixed.

I do however wonder if there are some other objects that don't have their hashkey reset in between tests, which causes more issues. This could be why the ngRepeat tests are failing (since ngRepeat heavily depends on correct hashkeys).

[caitp]

Yeah the ngRepeat failures are actually really basic, like, asserting that we'll throw on duplicate keys --- we throw the right thing, but we're asserting that the wrong hash key is used because we aren't expecting the module functions to be increase the iid number, so it's like 005 instead of 003 or something.

It would be easy to fix these tests, but I'm not sure there's a good way to robustly ensure that all uids get reset, at least not without letting angular-mocks override the hashKey function and get it to schedule the removal of a key from an object at the end of a test

[IgorMinar]

can you try that? on the angular test suite? decorate hashKey, keep track of all objects being hashed and clean them up at the end. (instead of using delete set the hashkey to null or undefined (deleting properties wrecks havoc in vms)).

if that works well, then we can expose hashKey as angular.$$hashKey so that we can override it from angular mocks for everyone not just angular test suite.

[caitp]

will give that a shot

[caitp]

Overriding hashKey in testabilityPatch to add an after() block which removes all hashKeys for objects/functions, with the simple implementation of this PR results in 14 test failures, all of which are assumptions about $rootScope's id or the ngRepeat dupe error tests. I think this might be a good approach to take, but I'm not super keen on module function uids knocking over other uids, because it will mean adding another module() function would change the behaviour of a test

[shahata]

@caitp @IgorMinar - I've updated this PR with what I think is the simplest solution to the issues we discussed here. In a nutshell, the idea is that each HashMap instance will maintain its uids separately and will not effect the global uid. This way @caitp concerns about module functions uids knocking over other uids are handled, and I think it is a bit cleaner. WDYT?

Regarding cleanup of $$hashKey from all objects and not only currentSpec.$modules - in my opinion doing it only for angular's test suite in testabilityPatch.js isn't really worth it and exposing it on angular.$$hashKey for angular-mocks to override might prove difficult since we will need to use angular.$$hashKey internally instead of hashKey. If you think it is worth it, I would be happy to send a PR for it as well.

[caitp]

ddescribe

[shahata]

oops :)

[shahata]

In theory, the fact that each HashMap maintains its own uids might cause trouble when using a single object instance in multiple HashMaps. This can be worked around pretty easily, I believe. @IgorMinar @caitp do you have any preference here?

[caitp]

that's probably not a likely scenario, since HashMap is only used by createInjector

[shahata]

It is also used by ngOptions, but you are probably right.

[IgorMinar]

I have a PR pending that changes uids to simple number counters (#7759) but let's keep things as they are here, I'll rebase my branch

[shahata]

I added another commit which lets the HashMap maintain a separate uid only if explicitly requested to (so only the injector HashMap has a different uid range)

[shahata]

@IgorMinar Okay, I'll squash the commits then (option 1 is already implemented here if u'd like to take a look)

[shahata]

Rebased since it seems #7759 was merged. Since uid can no longer be passed by reference into nextUid, I had to actually duplicate nextUid (one line) in HashMap. @caitp @IgorMinar - would be happy to hear if you have a better idea.

[shahata]

I could of course make uid an object with a value property, but I don't know if it is worth it.

[caitp]

the behaviour of this is very, very different from nextUid --- maybe that's okay, maybe it's not.

[shahata]

It is the same. @IgorMinar changed it in #7759

[caitp]

I don't think it's very good to give the hashMap its own "unique uid" without removing the hashKey from module functions / objects, though, because this will reset every time the injector is created (assuming the injector has an isolate uid), and it becomes possible for moduleFns to have duplicate uids and not be called

[caitp]

hmm, maybe it's not possible to reuse them... yeah maybe it's fine. -- wait, I'm wrong.

modules defined in beforeEach() would need to be cleaned up, otherwise they wouldn't get a new hashKey, and if a spec does this:

it("...", function() {
  // this never gets evaluated, because it would be reusing a hashkey already
  // loaded into the injector
  module(function() { ... });
  inject(function() { ... });
});

[shahata]

I am removing the hashKey... See angular-mocks.js

[caitp]

okay, good enough for now

[caitp]

This got the LGTM today, so lets check it in. I think this should go into stable too, since it's an injector fix.

But I'm going to make sure tests are passing first.