Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify local executions #115

Conversation

krmahadevan
Copy link

  • Ripped out local hub/node concept
  • Introduced a local driver factory that produces
    concrete browser web driver implementations for local
    runs.
  • Removed support for htmlunit in local executions
    because it doesn’t extend RemoteWebDriver.
  • Introduced a new config parameter to obtain
    the command to start a local node.
  • Removed out the capability matcher from client
    because now we don’t need it any more.
  • Removed the Hub class that we had duplicated from
    selenium codebase.

* Ripped out local hub/node concept
* Introduced a local driver factory that produces
concrete browser web driver implementations for local
runs.
* Removed support for htmlunit in local executions
because it doesn’t extend RemoteWebDriver.
* Introduced a new config parameter to obtain
the command to start a local node.
* Removed out the capability matcher from client 
because now we don’t need it any more.
* Removed the Hub class that we had duplicated from
selenium codebase.
@krmahadevan
Copy link
Author

@mach6
This pull request is basically aimed to be as a Proof of Concept to what I was suggesting when we were discussing in the PR #114

This is in no way complete, but basically should demonstrate the idea that I am trying to convey.

@fakrudeen78 @sebady

Please do chime in with your thoughts as well.

@sebady
Copy link

sebady commented Jul 9, 2015

My $0.02

I still believe that a locally started node and hub is a scenario we should try to support from SeLion client. I can't see a good reason to rip out this functionality and more importantly abandon this use case.

I see some benefit in having an execution environment that is somewhat "closer" to the one provided to clients by the Grid.

I also see this capability of driving a local browser as a good addition. I like the simplified setup provided here (using driver instances directly) as an addition instead of a replacement for the existing runlocally support.

So I would prefer we make this an addition at some point instead of replacement for the existing runlocal support.

@krmahadevan
Copy link
Author

@sebady

I hear you.

I still believe that a locally started node and hub is a scenario we should try to support from SeLion client. I can't see a good reason to rip out this functionality and more importantly abandon this use case.

Do you want to help me understand why ? If the functionalities were differing I would agree. But am sure you are also aware of the fact that the Grid/Node is just a relay setup that facilitates RMI calls into a remote JVM that runs in a different machine but under the hoods everything in Grid/Node falls back to still leveraging the actual browser implementations and doesn't do anything on their own. So what is the benefit of still clinging on to a locally started node and hub (which is a round about way of doing things) when it can be done directly .

I see some benefit in having an execution environment that is somewhat "closer" to the one provided to clients by the Grid.

What benefits are you talking about ? The execution environment that you refer to is not actually the execution environment per se but more on a RMI call facility that the Grid/Node gives you. The execution environment to me is ideally speaking the OS flavor + OS version + Browser flavor + Browser version.

So I would prefer we make this an addition at some point instead of replacement for the existing runlocal support.

This to me sounds way too redundant and there is no value add by us providing both of the options.

@fakrudeen78
Copy link
Contributor

@krmahadevan
I am fine with the approach. No server dependency in selion client.
One suggestion:
I don't think starting mobile node from selion gives any value addition to our users. So i think we can consider removing this feature. (user can use SeLion Server or respective mobile driver server)
@sundaramrajendran also mentioned the same in his comment #114 (comment)

@mach6
Copy link
Contributor

mach6 commented Jul 9, 2015

We should consider these options and suggestions after M5 and 1.0.0.. I don't want to rip out existing capabilities in M5

@fakrudeen78
Copy link
Contributor

My suggestion is lets do this before releasing 1.0.0, so that we will not
do major changes immediately after 1.0.0
And from the user perspective he won't feel any difference bec of this
change.

On Thu, Jul 9, 2015 at 11:43 AM, Doug Simmons notifications@github.com
wrote:

We should consider these options and suggestions after M5 and 1.0.0.. I
don't want to rip out existing capabilities in M5


Reply to this email directly or view it on GitHub
#115 (comment).

@mach6
Copy link
Contributor

mach6 commented Jul 9, 2015

We have a functioning, yet slightly inefficient, approach that works and has been working for us and our users (internally and externally) for a long time now.. We're not going to rip it out weeks before a release -- especially when there is already a list of things higher priority to complete beforehand. If 1.1.0 is nothing else but changing the way we launch web drivers locally, then that's perfectly acceptable.

@fakrudeen78
Copy link
Contributor

In my opinion this is not a big change set. And this PR and
#114 have almost same complexity. When
we are ready to take risk and include 114 before release why not we do the
same for this pull request.

On Thu, Jul 9, 2015 at 12:36 PM, Doug Simmons notifications@github.com
wrote:

We have a functioning, yet slightly inefficient, approach that works and
has been working for us and our users (internally and externally) for a
long time now.. We're not going to rip it out weeks before a release --
especially when there is already a list of things higher priority to
complete beforehand. If 1.1.0 is nothing else but changing the way we
launch web drivers locally, then that's perfectly acceptable.


Reply to this email directly or view it on GitHub
#115 (comment).

@mach6
Copy link
Contributor

mach6 commented Jul 21, 2015

FYI - I have started to incorporate this PR against the latest develop and will share a new branch which I started from a rebase of krmahadevan_LocalExecutionSupport as soon as something is shareable/working.

@krmahadevan
Copy link
Author

I am closing this Pull Request off, since I dont see any traction in my proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants