Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(ngMock): decorator that adds Scope#$countChildScopes and Scope#$countWatchers #9871

Closed
wants to merge 1 commit into from

Conversation

IgorMinar
Copy link
Contributor

When writing tests it's often useful to check the number of child scopes
or watchers within the current current scope subtree. Common use-case for advanced
directives is to test that the directive is properly cleaning up after itself. These
new methods simplify writing assertions that verify that child scopes were properly
destroyed or that watchers were deregistered.


NOTE for reviewers

  • This feature was asked for by Googlers, specifically @mprobst. Martin, please help with the review.
  • There is a PR by @lgalfaso that could be leveraged by this feature: perf($scope): Add a property $$watchersCount to scope #5799
  • I don't know if doing this as a mock decorator is the best solution because people might write unit tests that rely on this api being available in production, but it won't be. At the same time, this api should not be used by production code, so I don't want to have in under src/. What do others think?
  • Since this feature has no impact on production code, I'm ok shipping it as part of a 1.3.x release.

…countWatchers

When writing tests it's often useful to check the number of child scopes
or watchers within the current current scope subtree. Common use-case for advanced
directives is to test that the directive is properly cleaning up after itself. These
new methods simplify writing assertions that verify that child scopes were properly
destroyed or that watchers were deregistered.
$rootScopePrototype.$countChildScopes = countChildScopes;
$rootScopePrototype.$countWatchers = countWatchers;

// TODO: remove if Object.getPrototypeOF works on IE9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caitp
Copy link
Contributor

caitp commented Nov 1, 2014

I think people might want to use this in production, for things like live debuggers. But it's a fair bit of code, so maybe we don't want that.

Need to fix the doc annotations so that travis will run.

currentScope = pendingChildHeads.shift();

while (currentScope) {
count += currentScope.$$watchers ? currentScope.$$watchers.length : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this, you have two methods implementing the same BFS, with only a tiny variation (accumulate $$watchers.length vs accumulate number of visits). Not sure if it's worth it, but you could consider pulling that out. Might be roughly the same amount of code, but you wouldn't have to write the same set of tests twice.

@mprobst
Copy link
Contributor

mprobst commented Nov 2, 2014

Thanks Igor, this will fit my need. I think this should also be interesting for anybody else writing large / complex widget sets, like the Material Design folks (@naomiblack ?), to make sure you're not leaking scopes or watches.

}));


it('should correctly navigate complex scope tree', inject(function($rootScope) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to mention in the description that this tests the sibling navigation.

@caitp
Copy link
Contributor

caitp commented Nov 5, 2014

@petebacondarwin lets get this in so we can land lucas' /others prs mm?

@petebacondarwin
Copy link
Contributor

@IgorMinar - we actually need to use such functions in $compile tests but the way this is written makes it hard to access them - or am I missing something like being able to load the ngMock module in the tests?

@petebacondarwin
Copy link
Contributor

Oh, I take that back. It is available.

@petebacondarwin
Copy link
Contributor

I think that @mprobst is right when he notes that the two methods have a lot of overlap.
If we refactored this to expose a new method $walkScope, which would also allow us to do things like quickly check that all the scopes have been destroyed - see https://github.com/angular/angular.js/blob/master/test/ng/compileSpec.js#L5375-L5380

@caitp
Copy link
Contributor

caitp commented Nov 5, 2014

I honestly don't think it's worth worrying about

@petebacondarwin
Copy link
Contributor

Probably - bigger fish and all that.

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

Successfully merging this pull request may close these issues.

None yet

6 participants