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

Conversation

@juliemr
Copy link
Member

@juliemr juliemr commented Oct 7, 2015

Use Angular2's testability API, if present, when waiting
for stability or loading a page.

Closes #2396

@juliemr
Copy link
Member Author

juliemr commented Oct 7, 2015

@hankduan could you take a look at this one?

The current implementation only waits for one Angular2 application per page - the one found using config.rootElement. If it finds an angular2 app, it will not try to find or wait for an angular1 application. An alternative would be to wait for all angular apps, or to add the behavior as a configuration option, but this seemed like the simplest initial setup to me - let me know if you disagree.

Use Angular2's testability API, if present, when waiting
for stability or loading a page.

Closes angular#2396
@juliemr
Copy link
Member Author

juliemr commented Oct 7, 2015

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.

@juliemr
Copy link
Member Author

juliemr commented Oct 7, 2015

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

@hankduan
Copy link
Contributor

hankduan commented Oct 7, 2015

Anyway, the PR itself looks fine to me functionally. Just left some small suggestion.

@stalniy
Copy link
Contributor

stalniy commented Oct 7, 2015

why don't you want to implement #2460? Instead of adding if-s you can create a simple and clear interface using Strategy pattern which will allow to test any app.

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.

@juliemr
Copy link
Member Author

juliemr commented Oct 7, 2015

@stalniy I've added a note at #2460 with my ideas on that issue. It's a slightly longer term goal. Angular2 has not yet been released, but it is already using Protractor to run tests, and many people are beginning to experiment with it and we need to start growing an e2e testing solution there.

This allows waiting for all angular applications on the page, for
angular2 apps only.
@juliemr juliemr merged commit c5d37c2 into angular:master Oct 7, 2015
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.

5 participants