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

feat(driverProvider): Add useExistingWebDriver driver provider #4756

Merged
merged 1 commit into from
Aug 16, 2018

Conversation

andyjack
Copy link
Contributor

@andyjack andyjack commented Apr 2, 2018

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@andyjack
Copy link
Contributor Author

andyjack commented Apr 5, 2018

Please check my CLA!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Apr 5, 2018
@kevcenteno
Copy link

@qiyigg Any thoughts on this?

@andyjack
Copy link
Contributor Author

Hi, any chance of this PR being merged?

* It is responsible for setting up the account object, tearing it down, and
* setting up the driver correctly.
*/
import * as q from 'q';
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for a simple question, but why you can not use async ... await construction instead of q library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly copied the code from another driver provider, but I could redo my PR with async/await if that helps it getting merged. I don't exactly know what the house rules are for protractor code but I didn't see anything prohibiting q, fwiw.

Copy link
Contributor

Choose a reason for hiding this comment

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

The driver providers were written before Promise was available in Node.js.
Probably it's fine to use async/await for new driver provider, but I am not sure whether it will just work since some other folks tried to refactor them using regular promise, but it turns out not easy. Since all the other driver providers are using q promise, I am also ok with q promise for this one.

@qiyigg
Copy link
Contributor

qiyigg commented Aug 16, 2018

merged, thanks

@qiyigg qiyigg merged commit 7b08083 into angular:master Aug 16, 2018
@cnishina
Copy link
Member

Discussion on driverProviderUseExisting

So I'm working on this selenium upgrade process to 4. I understand the idea of this PR; however, I don't think this actually accomplishes the desired task. The selenium session should be passed to the constructor in 4.

class WebDriver {
  /**
   * @param {!(./session.Session|IThenable<!./session.Session>)} session Either
   *     a known session or a promise that will be resolved to a session.
   * @param {!command.Executor} executor The executor to use when sending
   *     commands to the browser.
   * @param {(function(this: void): ?)=} onQuit A function to call, if any,
   *     when the session is terminated.
   */
  constructor(session, executor, onQuit = undefined) {

Shown here:
https://github.com/SeleniumHQ/selenium/blob/master/javascript/node/selenium-webdriver/lib/webdriver.js#L636

We might already do this

The code provided looks and feels like if I was using an existing selenium server which is already defined here: https://github.com/angular/protractor/blob/master/lib/driverProviders/driverProvider.ts#L46 The only difference here that I can tell is that you are making the person writing the test also create the webdriver instance. Moving forward, attachToSession is being changed and we need to work on fixing the existing one.

@andyjack andyjack deleted the andyjack/use-existing-webdriver branch November 16, 2018 14:44
@andyjack
Copy link
Contributor Author

andyjack commented Nov 16, 2018

Hi @cnishina - you are correct, my intent with useExistingWebDriver was to make it for having regular ``selenium-webdriverjsand protractor use the same instance of aWebDriver`. Some more info here. I couldn't see any way in the protractor code of instantiating protractor where it wouldn't also create a new `WebDriver`. The decisioning for what provider is used based on protractor config is somewhat involved so adding a new case felt reasonable, rather than contorting the config to match the `attachToSession` provider (and I knew that `attachToSession` was already gone in selenium v4).

If there's a replacement for what the current attachToSession driver provider does in the v4 upgrade to protractor, that also allows one to instantiate protractor with an already-created driver, then removing useExisting sounds like a good idea to me.

Thank you btw for working on the upgrade 🎉

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.

6 participants