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

Support spawning additional browser instances #1537

Closed
wants to merge 1 commit into from
Closed

Support spawning additional browser instances #1537

wants to merge 1 commit into from

Conversation

jonhoo
Copy link

@jonhoo jonhoo commented Nov 18, 2014

Many end-to-end application tests require having multiple browser instances (windows/tabs/etc.) open at the same time so that functionality across clients can be tested. This patch implements a newInstance method on Protractor.prototype which returns a new Protractor wrapping a new instance of the underlying driver.

Drivers can advertise that they support this capability by exposing the newDriverInstance method (no arguments). This patch also implements this feature for the direct provider. Porting to other driver providers should be fairly straightforward, but has not been attempted.

Notes about the patchset:

  • Protractor now needs to have access to the driver provider, and not just the provider. It also needs access to certain parts of the config (from Runner) so that it can correctly set script timeouts. This has currently been solved by passing in the entire Runner to prevent an excessive number of arguments. Perhaps a better approach would be to make a config object where the necessary parameters (including baseUrl and rootEl) are contained.
  • Setting the script timeout is currently done by Runner (and now Protractor). Would it not be better for this to always be done by the driver? Or maybe even now by Protractor?
  • Should newDriverInstance be added to the driver provider interface overview?

This PR fixes #381.

@hankduan
Copy link
Contributor

How does this work in practice? Wouldn't the actions across the two browser be completely out of sync since they're using different drivers' control flows?

@juliemr
Copy link
Member

juliemr commented Nov 18, 2014

This is an automated message from the CLA checker script. Please sign CLA at https://developers.google.com/open-source/cla/individual Please make sure that the email associated with your PR is the same as the email you use to sign.

@juliemr juliemr added cla: no and removed cla: yes labels Nov 18, 2014
@jonhoo
Copy link
Author

jonhoo commented Nov 18, 2014

@hankduan: you're right, the additional browser instances are not synced at all with the main browser. Instead, you would write test code like this:

// ...
  it('should receive email subscriptions', function(done) {
    browser.get('apps/email.html#/' + email());

    var altBrowser = browser.newInstance('apps/email.html#/' + email() + '/compose');
    altBrowser.element(by.model('email.to')).sendKeys(email());
    altBrowser.element(by.id('sendEmail')).click();

    setTimeout(function() {
      expect(element.all(by.repeater('email in emails')).count()).toBe(1);
      done();
    }, 1000);
  });
// ...

@hankduan
Copy link
Contributor

Actually I was wrong about that. All drivers share the same control flow now that I read through webdriverjs' code again. So, in this example

  it('test', function() {
    browser.get(SOME_URL);
    for (var i = 0; i < 100; ++i) {
      element(by.model('abc')).sendKeys('123');
      element(by.id('def')).click();
    }
    var altBrowser = browser.newInstance(SOME_URL);
    for (var i = 0; i < 100; ++i) {
      altBrowser.element(by.model('abc')).sendKeys('123');
      altBrowser.element(by.id('def')).click();
    }
  });

because both browser and altBrowser schedules into the same control flow, altBrowser would not actually run until browser finishes running.

I think this is the behavior we want -- can't think of any reason to make multiple browsers act independent of each other since that makes the test indeterministic.

That being said, I still don't like the fact that browsers don't close on their own and that you need to include runner inside protractor. Let me think on this a bit more.

@jonhoo
Copy link
Author

jonhoo commented Nov 18, 2014

Hmm, yes, I think you're right; having the operations be serialized in the same control flow seems to be the expected behaviour.

With regards to browsers not closing on their own, the easy fix is to have Runner call browser.quit() instead of tearing down the driver directly. That way, the quit calls would cascade down the list of child instances so that all browsers are closed correctly. Then there would be no need for tests to explicitly close browser instances.

As to Runner being passed to Protractor, I agree that this is definitely not ideal. I don't feel like I know the Protractor code well enough to come up with a better alternative, so any input would be welcome.

@hankduan
Copy link
Contributor

I think to deal with the runner issue, we need to go about the PR from another way.
Specifically, allow a user to specify how many browser instances they want (default to 1) in the config. All the set up would still be done in the normal way, except duplicated as many times as the number of browser instances. In the test you would have browser.switchTo(index) in parallel with webdriver.

For the user this would mean having to define config.numOfBrowser before test begin and switching browser via browser.switchTo(index). There's another minor inconvenience in that all tests would instantiate config.numOfBrowser browsers even if they don't use it (i.e. if you mix interactive tests with non interactive ones). However, I feel like this is a better alternative than putting runner into protractor.js.

@jonhoo do you mind if I take this issue?

@jonhoo
Copy link
Author

jonhoo commented Nov 19, 2014

Hmm, I'm not entirely sure that's a particularly good API to work with though. First of all, it means you need to know how many browsers to spawn before the tests are started. In most cases, that might be fine, but for some more dynamic tests, the number of browser instances could vary.. Also, if you ever add a test to a suite, and that test needs some larger number of browsers you have to remember to increment config.numOfBrowsers, and all your other tests in that suite will now also have to run with that many browsers (although some of the windows might be unused).

Furthermore, I'm not sure I like the browser.switchTo(index) style API. There are a couple of reasons for this:

  • Many tests would probably need to start with something like browser.switchTo(0), as it's likely the different browsers are set up to point to different pages. The alternative would be to always navigate whatever browser window is currently active to the expected URL, but this a) still requires a call at the beginning of each test, and b) makes it harder to keep track of which browser index does what.
  • Using numerical indices to refer to different browser indices will probably become confusing pretty quickly as the number of browser instances increases. At the very least it would be nice to index them by some string name, but having a dedicated handle for each one would be even better; you always know exactly what instance you are doing an operation on.

FWIW, I'm happy for you to take over the issue.

@juliemr
Copy link
Member

juliemr commented Nov 20, 2014

Summarizing a conversation we had offline:

  • dynamically changing the number of browsers seems like a cool use case, but is probably a very very small number of legitimate users. I imagine that 99% of people who want to use multiple drivers at once want two and only two browsers. (Tell me if I'm wrong here).
  • The actual command name won't be browser.switchTo (since that's already taken as the API that switches between windows and tabs). It might look like browser.switchToDriver(1) or such.

@jonhoo
Copy link
Author

jonhoo commented Nov 20, 2014

While it's probably true that people will be using relatively few browser instances (personally, I currently need three, but I don't envision needing many more), my objection about having to switch between browsers instead of just performing operations on one browser object or the other still hold. Being explicit about what browser you want to perform an operation on is both less error-prone and more obvious to a person reading the code than the corresponding switchToDriver semantics.

I also feel very strongly that using numerical indices should be avoided if at all possible, as it makes it harder to recognize immediately which browser does what.

@googlebot
Copy link

CLAs look good, thanks!

@juliemr
Copy link
Member

juliemr commented Nov 20, 2014

I see what you're saying about the switchToDriver semantics, and I'm not thrilled about them either. It seems like the alternative is something like:

browser.get() // this is the 'default' or 0th browser
element() // this works of the 0th browser
var browser2 = magicBrowserInstancesGlobal(1);
element2 = browser2.element;
$2 = browser2.$;

Which is also messy.

@hankduan
Copy link
Contributor

implemented as described: #1548
switchToDriver can be messy if everything is done in a flat function. However, for people with tests involves complex scenarios like interaction, it is best to have a proper test structure (i.e. objects representing different user, etc, and this can deal with the switching in a clear way).

With regard to opening config.numDriver browsers for every test (as opposed to doing it dynamically), honestly I don't like it either, but the alternative is making the code structure really messy, and like Julie mentioned, compared to the number of people who will need this functionality, the tradeoff doesn't seem to be worth it.

@jonhoo
Copy link
Author

jonhoo commented Nov 20, 2014

Continued in #1548.

@jonhoo jonhoo closed this Nov 20, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Suggestion: Support multiple browser instances from single test
4 participants