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

feat(testability): add $testability service #8767

Closed
wants to merge 1 commit into from

Conversation

juliemr
Copy link
Member

@juliemr juliemr commented Aug 25, 2014

The $testability service is a collection of methods for use when debugging
or by automated testing tools. It is available globally through the function
angular.getTestability.
For reference, see the Angular.Dart version at
dart-archive/angular.dart#1191

@vojtajina
Copy link
Contributor

@juliemr I don't understand why this should be part of the angular core. I think this should be an additional module (such as ngResource, ngAnimate, etc) that you load when testing with Protractor. Neither do I see reason for adding public API angular.getTestability.

Am I missing something?

@juliemr
Copy link
Member Author

juliemr commented Aug 26, 2014

The getTestability was suggested by @chirayuk for consistency between angular.js and angular.dart.

I argue that this should be part of the core so that you can run tests against any version of your application (including production, maybe for monitoring). Testability can/will also be used by batarang and other debug tools, and it would be a pain to need a different environment to serve those for debugging. Telling people 'by the way, include this other script to be able to use our debugger or test' seems like a not great user experience.

@vojtajina
Copy link
Contributor

I absolutely agree that this code should be part of the angular repo so that it is "up-to-date" with each version of Angular.

Why can't this be a separate module, in the same way as ngAnimate or ngResource. You can add it to a page with minified Angular just fine. I think Protractor/Batarang could add that module automatically, in the same way it adds "config" module with disabling animations etc.

I can see it might be tricky for Protractor to know where to get specific version of the angular-testability module - the user running the tests would need to tell Protractor where the file is. I don't see how that is different to angular-mocks module which is not part of the core either.

@juliemr
Copy link
Member Author

juliemr commented Aug 26, 2014

I think angular-mocks is a slightly different situation because it cannot be included in angular-core, since it changes behavior. So, it's reasonable to tell users they need to do some extra setup to make mocks happen.

For testability, adding it to core increases the minified file by <1K (and this will decrease after #8430 lets us simplify the findBindings logic). It lets us avoid potential issues such as a scenario where one test hits different sites, potentially using different versions of Angular (an Angular-based login page is a common example). It makes everything 'just work' for someone writing e2e tests. I also recall that we'd like to reduce/eliminate the $browser service, and keeping $testability in core would allow us to move features of $browser there (they may require interaction with other services in core).

I'm open to the possibility of a separate module in a separate file, but I think that the benefits to the developer of including it in core outweigh the small size increase.

@vojtajina
Copy link
Contributor

I'm not worrying about the size that much. It's just that adding testing code into production feels wrong. This is also adding public APIs.

I agree that this makes it easy for developers. I think this is also "easy" solution for us. Maybe there is a solution that will make it easy for developers without shipping testing code in the core angular. I will try to think about it some more.

@@ -78,6 +78,7 @@
$SceDelegateProvider,
$SnifferProvider,
$TemplateCacheProvider,
$TestabilityProvider,
Copy link
Contributor

Choose a reason for hiding this comment

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

why make it a public api? I'd prefer to keep it private for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

What does private mean - just no ng-doc?

Copy link
Member Author

Choose a reason for hiding this comment

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

As we discussed, this also means making it $$testability. Done.

@juliemr
Copy link
Member Author

juliemr commented Aug 26, 2014

I've been thinking about that too. I think the reasons that adding testing code into production code feels wrong all don't apply to this service.

  • looks sloppy to users. Since testability is all bundled into one service, I don't think this is true.
  • security issue: there's nothing in testability that anyone can't do in the console.
  • file size: we just said we're not worried about this. (EDIT: not worried about the increase that this one causes)
  • slows down runtime: nothing here will be called unless it's done explicitly by the test/user.

I'll sleep on this as well.

@IgorMinar
Copy link
Contributor

just for the record: we do care about file size but there isn't that much code here :-)

@juliemr
Copy link
Member Author

juliemr commented Aug 27, 2014

After sitting on it I still feel like having this in core is the best experience for users and the best way to give us flexibility with how testability is implemented (e.g. maybe moving stuff from $browser into $testability). Any other ideas or concerns?

@juliemr juliemr force-pushed the testabilityapi branch 2 times, most recently from c9c1c11 to 09b6513 Compare August 27, 2014 22:48
*/
testability.findBindings = function(element, expression, opt_exactMatch) {
var bindings = [];
if (element.className.indexOf('ng-binding') !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

as much as I don't want to say this, this will break for SVG elements, because className is an SVGAnimatedString. As much as I don't want it to matter that it will break, people do use interpolation for SVG content attributes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, this will only be an issue if the user has set their root element to an svg element (seems very unlikely). In fact, I wonder if including the root element in the binding search is reasonable at all. I added this because without it one test fails, but that test was an odd case, where there was a {{stuff}} directly inside the <body>.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably just opt to use querySelectorAll anyways, if it's available

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, querySelectorAll only finds child elements - the idea here was to take a look at the root element itself. But I'm convincing myself that's a bad idea. I'm taking a look at just removing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you don't want to look at the root element.

There are alternatives: matches() (which might be msMatches(), mozMatches(), or webkitMatches(), etc), or just jqLiteHasClass for the root element being searched

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed! This has the advantage of making the code slightly smaller as well.

The $$testability service is a collection of methods for use when debugging
or by automated testing tools. It is available globally through the function
`angular.getTestability`.
For reference, see the Angular.Dart version at
dart-archive/angular.dart#1191
@juliemr
Copy link
Member Author

juliemr commented Aug 28, 2014

I have now rebased this on top of the changes to the debugging options submitted as #8802. This allows simpler logic for findBindings. I have also reverted changes to the example tests which added whitespace to by.binding, which is unnecessary after the testability api. The increase caused by this commit is now 275b minified and gzipped.

juliemr referenced this pull request in petebacondarwin/angular.js Aug 28, 2014
@juliemr
Copy link
Member Author

juliemr commented Aug 28, 2014

Merged as 85880a6

@jeffbcross
Copy link
Contributor

👍

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.

7 participants