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

Make controllers used with a bindToController directive easier to test #9425

Closed
meyertee opened this Issue Oct 4, 2014 · 31 comments

Comments

Projects
None yet
@meyertee

meyertee commented Oct 4, 2014

There doesn't seem to be a clear mechanism of unit-testing a controller that's used in a directive with bindToController: true. These controllers assume certain properties being set before the execution of the constructor.
I could build a mechanism myself using a temporary constructor etc. but it would be nice to have something in Angular itself.

I discovered the private boolean arg later in the $controller service that serves the purpose:

ctrlFn = $controller(MyCtrl, {
    $scope: scope,
}, true):
ctrlFn.instance.foo = "bar";
ctrl = ctrlFn();

Would you consider making that flag public and thus officially supported?
Or maybe add a helper to angular mock?

See also this Stack Overflow question.

@ernestoalejo

This comment has been minimized.

Show comment
Hide comment
@ernestoalejo

ernestoalejo Oct 4, 2014

+1. Testing bindToController should be simple, like the rest of Angular.

+1. Testing bindToController should be simple, like the rest of Angular.

@jeffbcross jeffbcross added this to the Backlog milestone Oct 9, 2014

@demisx

This comment has been minimized.

Show comment
Hide comment

demisx commented Nov 19, 2014

+1

@azevedo

This comment has been minimized.

Show comment
Hide comment

azevedo commented Dec 21, 2014

+1

@paulstefanday

This comment has been minimized.

Show comment
Hide comment

+1

@Epotignano

This comment has been minimized.

Show comment
Hide comment

+1

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Jan 19, 2015

Contributor

Whether you use the flag or not, it's not going to set up bindings for you, the compiler does that. Why not just compile some DOM, then the work is done for free

Contributor

caitp commented Jan 19, 2015

Whether you use the flag or not, it's not going to set up bindings for you, the compiler does that. Why not just compile some DOM, then the work is done for free

@Epotignano

This comment has been minimized.

Show comment
Hide comment
@Epotignano

Epotignano Jan 19, 2015

Can you write an example? there are not docs on how to test when use bindToController?

Can you write an example? there are not docs on how to test when use bindToController?

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Jan 19, 2015

Contributor

something like this

it('should populate template', inject(function($compile, $rootScope) {
  element = $compile('<div my-directive-with-controller cat-bindings="cats"></div>')($rootScope);
  // edit: use isolateScope() rather than scope(), since this is isolate-scopes-only for now
  newScope = element.isolateScope();
  ctrl = newScope.someId;

  expect(ctrl.catBindings).toBeUndefined();

  $rootScope.$apply('cats = [{name: "Bobby", weight: 6}, {name: "Parker", weight: 7 )]');

  // test that the controller's binding has the right stuff
  expect(ctrl.bindings).toEqual([
    { name: "Bobby", weight: 6 },
    { name: "Parker", weight: 7 }
  ]);

  table = element.find('table');
  rows = element.find('tr');

  // test that the template looks right
  expect(rows.eq(0).find('td').eq(0).text()).toBe('Bobby');
  expect(rows.eq(0).find('td').eq(1).text()).toBe('6.0 kilos');
  expect(rows.eq(1).find('td').eq(0).text()).toBe('Parker');
  expect(rows.eq(1).find('td').eq(1).text()).toBe('7.0 kilos');

  // okay, we're all done!
}));
Contributor

caitp commented Jan 19, 2015

something like this

it('should populate template', inject(function($compile, $rootScope) {
  element = $compile('<div my-directive-with-controller cat-bindings="cats"></div>')($rootScope);
  // edit: use isolateScope() rather than scope(), since this is isolate-scopes-only for now
  newScope = element.isolateScope();
  ctrl = newScope.someId;

  expect(ctrl.catBindings).toBeUndefined();

  $rootScope.$apply('cats = [{name: "Bobby", weight: 6}, {name: "Parker", weight: 7 )]');

  // test that the controller's binding has the right stuff
  expect(ctrl.bindings).toEqual([
    { name: "Bobby", weight: 6 },
    { name: "Parker", weight: 7 }
  ]);

  table = element.find('table');
  rows = element.find('tr');

  // test that the template looks right
  expect(rows.eq(0).find('td').eq(0).text()).toBe('Bobby');
  expect(rows.eq(0).find('td').eq(1).text()).toBe('6.0 kilos');
  expect(rows.eq(1).find('td').eq(0).text()).toBe('Parker');
  expect(rows.eq(1).find('td').eq(1).text()).toBe('7.0 kilos');

  // okay, we're all done!
}));
@ernestoalejo

This comment has been minimized.

Show comment
Hide comment
@ernestoalejo

ernestoalejo Jan 19, 2015

So if I'm not wrong the unit tests would be something like this now:

it('should have a default label', inject(function($compile, $rootScope) {
  element = $compile('<remove-button></remove-button>')($rootScope);
  newScope = element.isolateScope();
  ctrl = newScope.someId;

  newScope.$apply();

  expect(ctrl.label).toBe('Delete');
}));

it('should use the specified label', inject(function($compile, $rootScope) {
  element = $compile('<remove-button label="foo label"></remove-button>')($rootScope);
  newScope = element.isolateScope();
  ctrl = newScope.someId;

  newScope.$apply();

  expect(ctrl.label).toBe('foo label');
}));

So if I'm not wrong the unit tests would be something like this now:

it('should have a default label', inject(function($compile, $rootScope) {
  element = $compile('<remove-button></remove-button>')($rootScope);
  newScope = element.isolateScope();
  ctrl = newScope.someId;

  newScope.$apply();

  expect(ctrl.label).toBe('Delete');
}));

it('should use the specified label', inject(function($compile, $rootScope) {
  element = $compile('<remove-button label="foo label"></remove-button>')($rootScope);
  newScope = element.isolateScope();
  ctrl = newScope.someId;

  newScope.$apply();

  expect(ctrl.label).toBe('foo label');
}));
@elkorn

This comment has been minimized.

Show comment
Hide comment
@elkorn

elkorn Jan 19, 2015

I'm using this approach - seems to work all right.

elkorn commented Jan 19, 2015

I'm using this approach - seems to work all right.

@Epotignano

This comment has been minimized.

Show comment
Hide comment
@Epotignano

Epotignano Jan 19, 2015

Is working well for me.

Is working well for me.

@cesarandreu

This comment has been minimized.

Show comment
Hide comment
@cesarandreu

cesarandreu Feb 10, 2015

+1 for making later public.

You wanna test your controller's behavior, not the directive's side-effects.

+1 for making later public.

You wanna test your controller's behavior, not the directive's side-effects.

@SonofNun15

This comment has been minimized.

Show comment
Hide comment
@SonofNun15

SonofNun15 Mar 4, 2015

We have intentionally pulled almost all of our logic out of link functions and into directive controllers because controllers are much easier to test. You don't have to build a random DOM fragment and you can easily isolate all of the necessary parameters. Until now. So if we use bindToController we have to take a step back and return to building and parsing DOM fragments by hand, making setup and / or every test more complicated. There has to be a better way to do this... 😐

We have intentionally pulled almost all of our logic out of link functions and into directive controllers because controllers are much easier to test. You don't have to build a random DOM fragment and you can easily isolate all of the necessary parameters. Until now. So if we use bindToController we have to take a step back and return to building and parsing DOM fragments by hand, making setup and / or every test more complicated. There has to be a better way to do this... 😐

@SonofNun15

This comment has been minimized.

Show comment
Hide comment
@SonofNun15

SonofNun15 Mar 4, 2015

+1 for making later public

+1 for making later public

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Mar 4, 2015

Contributor

So what sort of testing API do you actually want? Like, how would you envision this working?

Contributor

caitp commented Mar 4, 2015

So what sort of testing API do you actually want? Like, how would you envision this working?

@SonofNun15

This comment has been minimized.

Show comment
Hide comment
@SonofNun15

SonofNun15 Mar 4, 2015

Before we build the controller, we currently set stuff on the scope object that we expect from the attribute bindings (isolated scope):

scope = $rootScope.$new();
scope.attribute = "expected param";
var controller = $controller("MyController", { $scope: scope });
return controller;

Now our test can verify that the controller set up its instance properties correctly based on the attribute bindings and / or test controller behavior via methods on the controller.

Before we build the controller, we currently set stuff on the scope object that we expect from the attribute bindings (isolated scope):

scope = $rootScope.$new();
scope.attribute = "expected param";
var controller = $controller("MyController", { $scope: scope });
return controller;

Now our test can verify that the controller set up its instance properties correctly based on the attribute bindings and / or test controller behavior via methods on the controller.

@SonofNun15

This comment has been minimized.

Show comment
Hide comment
@SonofNun15

SonofNun15 Mar 4, 2015

If we are using bindToController: true, this would ideally change to something like either:

var buildController = $controller("MyController", { }, true);
buildController.instance.attribute = "expected param";
return buildController();

Or, better yet something like:

var controller = $controller("MyController", { }, { attribute: "expected param" });
return controller;

If we are using bindToController: true, this would ideally change to something like either:

var buildController = $controller("MyController", { }, true);
buildController.instance.attribute = "expected param";
return buildController();

Or, better yet something like:

var controller = $controller("MyController", { }, { attribute: "expected param" });
return controller;
@SonofNun15

This comment has been minimized.

Show comment
Hide comment
@SonofNun15

SonofNun15 Mar 4, 2015

The root of the problem is that the bound attributes need to be set on the controller instance before the controller function is called.

The root of the problem is that the bound attributes need to be set on the controller instance before the controller function is called.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Mar 4, 2015

Contributor

@petebacondarwin what do you think? mock $controller service could have an extra parameter for setting up bindings, I guess?

The limitation is, they wouldn't be real bindings, and wouldn't result in any $watches or anything being set up. Which is great for some kinds of testing, but not so great for others

Contributor

caitp commented Mar 4, 2015

@petebacondarwin what do you think? mock $controller service could have an extra parameter for setting up bindings, I guess?

The limitation is, they wouldn't be real bindings, and wouldn't result in any $watches or anything being set up. Which is great for some kinds of testing, but not so great for others

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 4, 2015

Member

I like that idea. I guess that in most tests you would simply simulate watch driven changes from the outside.

Member

petebacondarwin commented Mar 4, 2015

I like that idea. I guess that in most tests you would simply simulate watch driven changes from the outside.

@caitp caitp modified the milestones: 1.4.0-beta.7 / 1.3.16, Backlog Mar 4, 2015

@caitp caitp self-assigned this Mar 4, 2015

@SonofNun15

This comment has been minimized.

Show comment
Hide comment
@SonofNun15

SonofNun15 Mar 4, 2015

In Angular 2.0, how will components initialize based on property bindings (when necessary)? Will property bindings be available as constructor parameters somehow, or will some component initialization need to occur after the constructor call completes?

In Angular 2.0, how will components initialize based on property bindings (when necessary)? Will property bindings be available as constructor parameters somehow, or will some component initialization need to occur after the constructor call completes?

@SonofNun15

This comment has been minimized.

Show comment
Hide comment
@SonofNun15

SonofNun15 Mar 4, 2015

This concept is very important for cases like:

<my-component [read-only]="true" [make-color]="blue" />

Highly useful to use attribute bindings, property bindings, etc to allow customization of the component by the consumer.

This concept is very important for cases like:

<my-component [read-only]="true" [make-color]="blue" />

Highly useful to use attribute bindings, property bindings, etc to allow customization of the component by the consumer.

@SonofNun15

This comment has been minimized.

Show comment
Hide comment
@SonofNun15

SonofNun15 Mar 4, 2015

Sorry, probably a bit off topic here, but very related to the use case that caused this issue.

Sorry, probably a bit off topic here, but very related to the use case that caused this issue.

caitp added a commit to caitp/angular.js that referenced this issue Mar 4, 2015

feat(ngMock): allow mock $controller service to set up controller bin…
…dings

Adds a new mock for the $controller service, in order to simplify testing using the
bindToController feature.

```js
var dictionaryOfControllerBindings = {
  data: [
    { id: 0, phone: '...', name: '...' },
    { id: 1, phone: '...', name: '...' },
  ]
};

// When the MyCtrl constructor is called, `this.data ~= dictionaryOfControllerBindings.data`
$controller(MyCtrl, myLocals, dictionaryOfControllerBindings);
```

Closes #9425

caitp added a commit to caitp/angular.js that referenced this issue Mar 4, 2015

feat(ngMock): allow mock $controller service to set up controller bin…
…dings

Adds a new mock for the $controller service, in order to simplify testing using the
bindToController feature.

```js
var dictionaryOfControllerBindings = {
  data: [
    { id: 0, phone: '...', name: '...' },
    { id: 1, phone: '...', name: '...' },
  ]
};

// When the MyCtrl constructor is called, `this.data ~= dictionaryOfControllerBindings.data`
$controller(MyCtrl, myLocals, dictionaryOfControllerBindings);
```

Closes #9425

@caitp caitp closed this in d02d058 Mar 6, 2015

caitp added a commit that referenced this issue Mar 6, 2015

feat(ngMock): allow mock $controller service to set up controller bin…
…dings

Adds a new mock for the $controller service, in order to simplify testing using the
bindToController feature.

```js
var dictionaryOfControllerBindings = {
  data: [
    { id: 0, phone: '...', name: '...' },
    { id: 1, phone: '...', name: '...' },
  ]
};

// When the MyCtrl constructor is called, `this.data ~= dictionaryOfControllerBindings.data`
$controller(MyCtrl, myLocals, dictionaryOfControllerBindings);
```

Closes #9425
Closes #11239
@meyertee

This comment has been minimized.

Show comment
Hide comment
@meyertee

meyertee Mar 6, 2015

Fantastic, thanks!

meyertee commented Mar 6, 2015

Fantastic, thanks!

@SonofNun15

This comment has been minimized.

Show comment
Hide comment

👍

@demisx

This comment has been minimized.

Show comment
Hide comment
@demisx

demisx Mar 6, 2015

👍 This is what it was meant to be.

demisx commented Mar 6, 2015

👍 This is what it was meant to be.

@cesarandreu

This comment has been minimized.

Show comment
Hide comment

👍

hansmaad pushed a commit to hansmaad/angular.js that referenced this issue Mar 10, 2015

feat(ngMock): allow mock $controller service to set up controller bin…
…dings

Adds a new mock for the $controller service, in order to simplify testing using the
bindToController feature.

```js
var dictionaryOfControllerBindings = {
  data: [
    { id: 0, phone: '...', name: '...' },
    { id: 1, phone: '...', name: '...' },
  ]
};

// When the MyCtrl constructor is called, `this.data ~= dictionaryOfControllerBindings.data`
$controller(MyCtrl, myLocals, dictionaryOfControllerBindings);
```

Closes #9425
Closes #11239
@HipsterZipster

This comment has been minimized.

Show comment
Hide comment

netman92 added a commit to netman92/angular.js that referenced this issue Aug 8, 2015

feat(ngMock): allow mock $controller service to set up controller bin…
…dings

Adds a new mock for the $controller service, in order to simplify testing using the
bindToController feature.

```js
var dictionaryOfControllerBindings = {
  data: [
    { id: 0, phone: '...', name: '...' },
    { id: 1, phone: '...', name: '...' },
  ]
};

// When the MyCtrl constructor is called, `this.data ~= dictionaryOfControllerBindings.data`
$controller(MyCtrl, myLocals, dictionaryOfControllerBindings);
```

Closes #9425
Closes #11239
@lokeshjainRL

This comment has been minimized.

Show comment
Hide comment
@lokeshjainRL

lokeshjainRL May 12, 2016

@ernestoalejo

What is someId?

in the snippet.

newScope = element.isolateScope();
  ctrl = newScope.someId;
``

@ernestoalejo

What is someId?

in the snippet.

newScope = element.isolateScope();
  ctrl = newScope.someId;
``
@ernestoalejo

This comment has been minimized.

Show comment
Hide comment
@ernestoalejo

ernestoalejo May 12, 2016

@lokeshjainRL I copied it from the other comment. Anyway this issue was closed later with a much easier approach:

describe('buttons.RemoveButtonCtrl', function () {
  var ctrl, $scope;

  beforeEach(inject(function ($rootScope, $controller) {
    scope = $rootScope.$new();

    ctrl = $controller('xxCtrl', {
      $scope: scope,
    }, {
      label: 'foo'
    });
  }));

  it('should have a label', function () {
    expect(ctrl.label).toBe('foo');
  });
});

(from http://stackoverflow.com/questions/25837774/bindtocontroller-in-unit-tests)

Pass the initial scope you want to test defaults against as the third argument to $controller.

ernestoalejo commented May 12, 2016

@lokeshjainRL I copied it from the other comment. Anyway this issue was closed later with a much easier approach:

describe('buttons.RemoveButtonCtrl', function () {
  var ctrl, $scope;

  beforeEach(inject(function ($rootScope, $controller) {
    scope = $rootScope.$new();

    ctrl = $controller('xxCtrl', {
      $scope: scope,
    }, {
      label: 'foo'
    });
  }));

  it('should have a label', function () {
    expect(ctrl.label).toBe('foo');
  });
});

(from http://stackoverflow.com/questions/25837774/bindtocontroller-in-unit-tests)

Pass the initial scope you want to test defaults against as the third argument to $controller.

@lokeshjainRL

This comment has been minimized.

Show comment
Hide comment
@lokeshjainRL

lokeshjainRL May 12, 2016

@ernestoalejo Thanks for pointing to the resources. But this approach is not when you have complex html with it when you need access to the input element and forms. I prefer solution from your comment.
I Figured that someId.

In directive definition

return{
bindToController:{
},
scope:{},
controller: 'directiveController',
controllerAs; 'someId'
}

That controllerAs attribute is the someId. in Your comment.

@ernestoalejo Thanks for pointing to the resources. But this approach is not when you have complex html with it when you need access to the input element and forms. I prefer solution from your comment.
I Figured that someId.

In directive definition

return{
bindToController:{
},
scope:{},
controller: 'directiveController',
controllerAs; 'someId'
}

That controllerAs attribute is the someId. in Your comment.

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