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

$rootScope reference has been destroyed by angular-mocks #154

Open
aciccarello opened this issue Sep 20, 2016 · 8 comments
Open

$rootScope reference has been destroyed by angular-mocks #154

aciccarello opened this issue Sep 20, 2016 · 8 comments

Comments

@aciccarello
Copy link
Collaborator

TLDR;

Unit tests don't run ngOnChanges correctly if they are run after another test because the $rootScope reference was destroy.

Details

In my unit tests I am using the $compile service to create my component and check the resulting DOM. After each test angular-mocks destroys the $rootScope service to clean up the test (introduced in angular/angular.js#13433). However, angular does not invoke the directiveControllerFactory again so when future tests run $apply() in the _flushOnChangesQueue() function, it is calling a noop. This is evidenced by the ngOnChanges() not be called again.

When debugging this issue, I can run two copies of the same exact test and the first instance would pass and the second would fail. I will try creating an example unit test to demonstrate.

@aciccarello
Copy link
Collaborator Author

Here is a plnkr demonstrating the failed test. Tests 1 & 2 are exactly the same but test 2 fails. In the console you can see that ngOnChanges is not called a second time.

https://plnkr.co/edit/sS2eau?p=preview

Component

import { Component, Input } from 'ng-metadata/core';

@Component({
  selector: 'my-app',
  template: `<h1>Angular 1 App <small>with ng-metadata!</small></h1>
<div>{{ $ctrl.internalProp }}</div>`
})
export class AppComponent {
  @Input() prop = 'default property value';
  internalProp: string;

  ngOnChanges(changes) {
    console.log('ngOnChanges was called with: ' + angular.toJson(changes));
    this.internalProp = this.prop;
  }
}

Component spec

import './main';
import { bundle } from 'ng-metadata/core';
import { AppComponent } from './app.component';

describe('AppComponent', () => {
  // This test will work because it is run first
  it('should update the text', () => {
    console.log('test 1');
    angular.mock.module(bundle(AppComponent).name);
    let {element, $scope} = compile();
    $scope.value = 'My new value';
    $scope.$apply();
    expect(element[0].textContent).toContain('My new value');
  });

  // Same test but it fails
  it('should update the text the second time around', () => {
    console.log('test 2');
    angular.mock.module(bundle(AppComponent).name);
    let {element, $scope} = compile();
    $scope.value = 'My new value';
    $scope.$apply();
    expect(element[0].textContent).toContain('My new value');
  });
});

@JamesHenry
Copy link
Member

Any thoughts on this @Hotell?

@Hotell
Copy link
Member

Hotell commented Oct 15, 2016

sorry was busy with non Angular things lately. Need take a look. Sry for delay @aciccarello

@Yann77
Copy link

Yann77 commented Oct 27, 2016

Same problem here... onChanges is not call on a sequence of tests.
Any news ?

@Yann77
Copy link

Yann77 commented Oct 27, 2016

Working work around proposed by
aciccarello on ng-party slack channel:

This assumes the component is the top level element.
If you have nested components you'd have to manually call ngOnUpdate on those too ;-(

function manuallyCallOnUpdate(element: angular.IAugmentedJQuery,  changes: SimpleChanges) {
  let componentScope = <any> angular.element(element[0].children.item(0)).scope();
  componentScope.$ctrl.ngOnChanges(changes);
  componentScope.$apply();
}

I updated his example so I can pass a SimpleChanges object as parameter.

@otijhuis
Copy link

otijhuis commented Apr 3, 2017

We're running into the same problem. For now we're working around it by doing $rootScope.$destroy = () => angular.noop();. Not the best solution but at least it saves us from writing specific test code to work around the issue.

@aciccarello
Copy link
Collaborator Author

@Hotell I'd like to see this fixed to improve testing but I'm afraid to touch it because I don't want to ruin production performance for a unit testing fix.

Right now, I think what needs to happen is that there needs to be a check to see if $rootScope has been destroyed. That way you don't need to take the performance hit of calling the injector every time but you can still run it in tests. Does that make sense? If so I can try to put together a PR.

I'm currious what the comment about something being made a singleton means in https://github.com/ngParty/ng-metadata/blob/master/src/core/change_detection/changes_queue.ts#L1. Is that referring to the ChangesQueue class? Trying to figure out if that would affect the implementation.

@eric-simonton-sama
Copy link

This took a lot of effort to track down, but once I got close, search terms got me to this issue. Thanks @aciccarello!

The workaround from @otijhuis fixed our tests that were failing, but caused new ones to fail (because of the way a 3rd part lib was using $rootScope). This secret sauce got our suite to pass again:

$rootScope.$destroy = () => $rootScope.$broadcast('$destroy');

Or, with our full setup (and in coffeescript):

# 1 wierd trick to set global configuration
beforeEach module 'ng', ($provide) ->
  $provide.decorator '$rootScope', ($delegate) ->
    $delegate.$destroy = -> $delegate.$broadcast('$destroy')
    $delegate

However, this feels very hacky, and makes me uncomfortable wondering what may pop up later as our code changes, including as we transition more of it to ng-metadata. (So far we have transitioned 2 components). I hope a real fix for this issue can be figured out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants