-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(lib): add support for waiting for angular2 #2586
Conversation
|
@hankduan could you take a look at this one? The current implementation only waits for one Angular2 application per page - the one found using |
Use Angular2's testability API, if present, when waiting for stability or loading a page. Closes angular#2396
|
Hm, actually after thinking about this for a minute longer - at least when testing the angular2 example applications, there's a lot of cases where the default behavior that we want is waiting for all ng2 apps. That way, we don't have to specify a different selector every time. I'm working on a modification to do that instead. |
|
OK, the branch has been updated with the addition of a new configuration option to wait for all angular2 applications on the page. This requires only one change to the config file. Let me know what you think of option name, it's kinda long. |
lib/clientsidescripts.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something, but wouldn't all ng1 tests fail right now? I guess by default the rootSelector is 'body' or something, but someone could have easily accidentally called 'useAllAngularAppRoots' for ng1. Maybe a safeguard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, the root selector gets defaulted to 'body' in the constructor for a Protractor instance. How would someone set useAllAngularAppRoots to true accidentally?
Note that if the rootSelector were somehow the empty string here already, this would fail at querySelector because it's not a valid css selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By accidentally I mean if they specify that in their config. The config name itself doesn't mention anything about ng2, and a lot of people don't read the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. Changed the name so that it's harder to mess up.
|
Anyway, the PR itself looks fine to me functionally. Just left some small suggestion. |
|
why don't you want to implement #2460? Instead of adding In case dev want to test both Angular1 and Angular2 apps he can use Decorator pattern or smth like composition between 2 strategies (Angular1 and Angular2). Angular2 haven't been released yet (just alpha), so we have time to implement this in transparent way. |
This allows waiting for all angular applications on the page, for angular2 apps only.
Use Angular2's testability API, if present, when waiting
for stability or loading a page.
Closes #2396