Fix ngMock window.inject() error stack trace reporting on repeated or non-initial injection function calls for the master branch (issue #13594)

[jurko-gospodnetic]

Fixes & adds tests for issue #13594 for the angular master branch.

Does the same work as pull request #13597 does on the angular 1.4.x branch.

window.inject() function updates stack trace information for Error objects thrown from injection functions passed to it with stack trace for the window.inject() calling location. However this functionality was broken and did not work correctly in the following cases:

• when the same injection function gets called multiple times
• when more than one injection function is passed to a single window.inject() call and a non-initial one throws an Error

This pull request actually builds on and thus includes pull request #13593, as newly added test specs are integrated into the test structure set up by the base pull request. That's why you should first review & merge that pull request, or ignore its commits when reviewing this one.

[Narretz]

Hi, I took a quick look at your PRs. Looking good so far. However, with all these commits it's hard to see what exactly belongs together What I would like to see is one commit per fix - that means the commit includes the fix, the tests and any refactorings. If you can create a new PR for the master branch with these changes, that would be great.

[jurko-gospodnetic]

Pull request #13593, that this one is based on, has only two commits:

• a5a913a - making the fix
• e3eb4bd - adding tests

So that one should be reviewed & merged first.

What is left for this pull request are:

• 1 test refactoring commit (0c1b078) intentionally split up to make it easier to review
• 1 fix commit (1c238a1)
• 3 comment adding commits (9b635bb, 3244cf9 & 7ad5695)
• 2 test adding commits (563ca46 & 4e5b9d7), each adding a single test related to this fix

Now, if you really feel that would help in some way, we can easily just squash it all together into:

• 1 commit for pull request Fix ngMock window.inject() stack trace reporting on PhanthomJS for master branch #13593 (from its 2 commits)
• 1 commit for this pull request (from its 7 commits)

though you can see the effects of all of these squashed together easily on the Files changed tab already, and I personally find it easier to analyse the changes later on if needed when they are split up into such separate, stand-alone & commented commits.

In the end - it is just one fix per pull request as it is now.

[Narretz]

I've taken another look at this. Sorry for the slow turnaround. The problem in #13593 I understand, but for these changes I can't really see the real world scenario where it would happen that you have multiple calls to inject() inside a single spec ( usually the spec wraps a single it fn, so you only have one inject per spec). I would like to see a real-world example of this being a problem first before merging it.
In other words, the current tests don't really reflect how this kind of inject calling behavior would happen in a real test.

[jurko-gospodnetic]

This pull request does not really introduce anything new into angular code. It only corrects handling that is already there (one line change removing too early clean-up) and adds missing tests for it. 😄

Here are a couple of scenarios in which I often see multiple inject() calls made within the context of a single test spec run:

Scenario 1

As a project's test suite grows, I find that it is often the case where some test utility functions get created. And some of them make use of specific angular-based services that they need to inject. In such cases it is typically useful to have that test utility perform the inject internally instead of requiring that the caller pass it in explicitly. And when you have that tool, and use it in your test, you easily get in a situation where you make an inject() call in the test call itself and a separate one inside the test utility function.

Scenario 2

One test code organization patterns I've often encountered (and which is actually real close to the example given in angular docs here) is this:

describe('some test suite', function () {
    var myService;

    beforeEach(inject(function (_myService_) {
        myService = _myService_;
    }));

    it('some spec', function () {
        ...make use of myService...
    });

    it('another spec', inject(function (anotherService) {
        ...make use of both myService & anotherService...
    }));
});

But really, even without considering such scenarios, the behaviour as implemented right now is inconsistent and seems to say - doing this is allowed, but it is not allowed, or rather, you can do it, and it will most likely work, but possibly won't do exactly what you'd expect, and we don't define or document what will happen. 😄

Saying I don't want to test using inject() multiple times in a single test spec or passing in multiple functions to a single inject() call begs the question - why is that supported then anyway? Why does inject() explicitly allow for & handle passing in multiple functions to it? An alternative to this patch would be reporting an error if an inject() call is made multiple times within the context of running a single test spec, or if multiple functions are passed into a single inject() call, which is a breaking change I'd really be less comfortable with than just testing what is already there.

As for the tests in this pull request, they have intentionally been prepared so they are each self-contained in a single it() call, but they can easily be refactored to use one of the patterns above (or some other), although I don't really see the point - they are supposed to test that multiple calls to inject() or calls to inject() with multiple functions getting passed in work correctly, and that's what they do clearly enough as they are right now.

[Narretz]

The second scenario makes perfect sense. I just had a hard time visualizing the setup from the abstract tests. While trying to recreate the scenario, I've come across another issue. In the following plnkr, I have the example setup you provided:
http://plnkr.co/edit/0edB849phWov4cRJCf9Q?p=preview

two specs, one inject in a beforeEach and another one in the second spec. Both inject the same service that is throwing on a fn that is called inside the spec. However, in the first spec, we don't get the inject part in the stacktrace (so no inject location is provided), but in the second spec we have it. From what I understand we should have the inject stack trace in both cases. Do you agree?

I have included your fix in the plnkr, so this is an issue that happens with your fix too.

[jurko-gospodnetic]

Busy atm, but certainly curious and will take a look. If you like we can sync up over Skype (my login there is jurkog) or hangout or whatever and take a look at this together over the weekend.

[Narretz]

@jurko-gospodnetic Sry, I didn't have time on the weekend, but we can sync on the coming weekend if you want

[jurko-gospodnetic]

Cool, just contact me over Skype when you're online. 👍

[jurko-gospodnetic]

@Narretz I took a look at that plunkr you left a link to, and I don't really see anything wrong with the behaviour shown.

However, in the first spec, we don't get the inject part in the stacktrace (so no inject location is provided), but in the second spec we have it. From what I understand we should have the inject stack trace in both cases. Do you agree?

Nope. The behaviour seems correct. The first spec does not fail inside any service injection function, while the second one does. Also, the behaviour presented in that plunkr does not really have anything to do with the changes done in this pull request as neither do any of the failing/throwing service injection functions get called multiple times nor do any of the inject() calls get passed multiple service injection callbacks.

There are actually two inject() calls done there:

1. The first inject() call made outside any specific test spec context, at the time when jasmine is still collecting all the test suites & specs
   1. it does not actually do any injections at that time, but instead returns a function that will do the myService injection and store the injected service in the myService variable, once it gets called
   2. that function is passed on into beforeEach() to have jasmine call it automatically at the start of each of our test specs
   3. since that service injection never fails, this whole initial inject() call is pretty much of no concern to us
   4. note that this constructed service injection function thus gets called twice - once for each test spec, and so, had it failed without the patch prepared in this pull request, its call location information would not have been included in the call stack information displayed for the second test case
2. The second inject() call also made outside any specific test spec context, at the time when jasmine is still collecting all the test suites & specs
   1. it does not actually do any injections at that time, but instead returns a function that will do the $q injection and then make the failing myService.get() call
   2. that function is passed on into the it() call as the implementation function for our second test case, to be called by jasmine at the appropriate time

and here is a more detailed description of what happens when each of our failing test specs get run:

1. The first failing test spec makes no additional inject() calls in it

   1. it just calls the injected service function myService.get(), which throws an Error

   2. the call stack presented for this test spec seems correct and lists:

      this.get@http://run.plnkr.co/U1KJKxif15akUge5/app.js:9:25

      service function making the actual Error throw

      @http://run.plnkr.co/U1KJKxif15akUge5/appSpec.js:39:15

      point in our test case function where we called the myService.get() function

      attemptSync@http://cdnjs.cloudflare.com/ajax/libs/jasmine/2.2.1/jasmine.js:1741:9
      QueueRunner.prototype.run@http://cdnjs.cloudflare.com/ajax/libs/jasmine/2.2.1/jasmine.js:1729:9
      QueueRunner.prototype.execute@http://cdnjs.cloudflare.com/ajax/libs/jasmine/2.2.1/jasmine.js:1714:5
      ...

      the rest of jasmine's infrastructural kludge needed to run our test spec function

   3. there is no additional call stack information for any inject() call because the test failure did not occur inside any service injection function

2. The second failing test spec also makes no additional inject() calls, and instead just calls our previously registered service injection function (constructed by the second inject() call)

   1. the injected $q service never gets used for anything and so is of no concern to us

   2. the call to myService.get() throws an error and we get the expected call stack listed:

      this.get@http://run.plnkr.co/U1KJKxif15akUge5/app.js:9:25

      service function making the actual Error throw

      @http://run.plnkr.co/U1KJKxif15akUge5/appSpec.js:44:17

      point in our test case function where we called the myService.get() function

      invoke@https://code.angularjs.org/snapshot/angular.js:4658:16
      workFn@http://run.plnkr.co/U1KJKxif15akUge5/angular-mocks.js:2832:11

      angular's service injection function that got returned by the second inject() call - remember, that is what we registered as our test spec's implementation function, and the fact that there are two layers in the stack for it is just an internal angular.js detail as it actually creates another internal wrapper for that function

      attemptSync@http://cdnjs.cloudflare.com/ajax/libs/jasmine/2.2.1/jasmine.js:1741:9
      QueueRunner.prototype.run@http://cdnjs.cloudflare.com/ajax/libs/jasmine/2.2.1/jasmine.js:1729:9
      QueueRunner.prototype.execute@http://cdnjs.cloudflare.com/ajax/libs/jasmine/2.2.1/jasmine.js:1714:5
      ...

      the rest of jasmine's infrastructural kludge needed to run our test spec function

   3. plus we get additional call stack information appended to the end (separated by a new-line) identifying the inject() call that constructed the service injection function inside which our error occurred:

      angular.mock.inject@http://run.plnkr.co/U1KJKxif15akUge5/angular-mocks.js:2791:25

      point inside angular.js's inject() implementation where this call stack was actually collected, when we made the inject() call in question (we could tweak the displayed stack to remove this line if needed, but that was not done in the original code and thus is out of scope for this specific pull request)

      @http://run.plnkr.co/U1KJKxif15akUge5/appSpec.js:42:67

      point in our test code where we made the inject() call that constructed the failing service injection function (in this case it has been done inline with the jasmine it() call defining the test spec in question so there is not much real use in this inject() call stack, but it would have come in useful had we called constructed the service injection function earlier and possibly reused it in different test specs)

      addSpecsToSuite@http://cdnjs.cloudflare.com/ajax/libs/jasmine/2.2.1/jasmine.js:725:9
      Env/this.describe@http://cdnjs.cloudflare.com/ajax/libs/jasmine/2.2.1/jasmine.js:695:7
      jasmineInterface.describe@http://cdnjs.cloudflare.com/ajax/libs/jasmine/2.2.1/jasmine.js:2969:14
      ...

      jasmine's plumbing for collecting the list of all the defined test suites & specs, as a part of which our inject() call was made

To see the difference with or without the patch applied, you might want to replace the second test case with something like this:

describe('Mercury', function () {
    beforeEach(inject(function($q) {
        service.get();
    }));

    it('Venus', function () {});
    it('Earth', function () {});
});

Which, without the patch applied, should make:

1. the Venus test spec display the inject() call location information
2. the Earth test spec NOT display the inject() call location information

while with the patch applied, both should correctly display the inject() call location information