Allow for customisation of TestSlot #3431

krmahadevan · 2017-01-28T17:05:41Z

The current implementation does not let downstream
consumers of the Grid provide a customised way of
injecting a TestSlot.
This is very useful when one needs to build an On-Demand Grid,
wherein there are no real proxies, but a ghost proxy stands
for the real proxy and based on new session requests,
nodes are spun off.

Fixed this by defining a method for instantiating a
TestSlot so that downstream RemoteProxy implementations
can choose to provide their own customised TestSlots.

Also enhanced the SeleniumProtocol enum to house all
logic related to forming the protocol and the path
from a desired capability into it.

By placing an X in the preceding checkbox, I verify that I have signed the Contributor License Agreement

krmahadevan · 2017-01-28T17:07:00Z

ping @lukeis - Can you please help vet this out ?

lukeis · 2017-02-01T05:06:35Z

java/server/src/org/openqa/grid/common/SeleniumProtocol.java

+  private void computePath(DesiredCapabilities capability) {
+    String localPath = (String) capability.getCapability(PATH);
+    if (localPath != null) {
+      this.path = localPath;


i think this is pretty dangerous, you're changing the enum's value of path. enums are static, so you're changing the path for every instance of the protocol type.

I'm imagining a hub that has multiple nodes with different paths.... if one changes the path, then all subsequent will have that new path, rather than the default /wd/hub path that they might assume.

@lukeis - Thanks for catching this. I completely missed that part. I will fix this.

@lukeis - I have fixed this and also added a unit test as well.

lukeis · 2017-02-01T05:10:53Z

java/server/src/org/openqa/grid/internal/RemoteProxy.java

+   * @param capabilities - the type of test the client is interested in performing.
+   * @return - The entity on a proxy that can host a test session.
+   */
+  default TestSlot newTestSlot(SeleniumProtocol protocol, Map<String, Object> capabilities) {


it's like you're trying to make a TestSlot factory so you can override it. I'm happy to have selenium provide a mechanism for that, just not sure this is the way I'd approach it. If you don't want to make another Factory class, maybe getTestSlotClass? I'm not sure....

@mach6 have any suggestions?

If we resort to a getTestSlotClass() wherein we are returning the actual test slot class, I thought that would bring in reflection (because now the proxy would have to instantiate the class) and we also would need to add additional checks which figures out if the class that was provided satisfies the isAssignableFrom() clause and all that. That was why I thought I would keep it simple by just defining a default method in the RemoteProxy interface, and any one specifically looking for customizing TestSlots can choose to explicitly override that method from their DefaultRemoteProxy extension. But please do let me know if there are other options that can be considered so that I can explore that and get this sorted out.

@mach6 - Please let me know your suggestions if any. I would like to wrap this PR up, so am trying to find out what else is pending from my side.

ping @mach6 . Any updates on this ?

ping @lukeis - If there are no updates from @mach6 , please let me know what more is needed from my side to get this merged.

I'd like to see a test that exercises the changes to TestSlot, overriding the default behavior.

@mach6

I'd like to see a test that exercises the changes to TestSlot, overriding the default behaviour.

What exactly would we be asserting in these tests ? A downstream user is ideally speaking going to be sub-classing TestSlot and then work with that sub-class by accessing it via the TestSession. Can you please elaborate the unit test requirements here ?

ping @mach6

Shouldn't you be able to create a DummyTestSlot for the purposes of a functional test?

@mach6 - Yes I very much can. But I would like to understand the intent of the functional test. It can at the max just do an instanceof check in the assertion. So can you please help me understand what would the test actually be doing as a value add. There is no change in code. The only thing that has been done in this PR is to move the instantiation of the TestSlot to an interface (which has been given a default behaviour as well)

mach6 · 2017-02-08T22:02:01Z

java/server/src/org/openqa/grid/common/SeleniumProtocol.java

+    }
+  }
+
+  public String getPath(Map<String, Object> capabilities) {


It seems inconsistent to me that getProtocol takes DesiredCapabilities and getPath takes a Map<String, Object>. Can they both take DesiredCapabilities as the original private code (in BaseRemoteProxy) that this was moved from did?

Also, now that these methods are public, I'd like to suggest names of

public static SeleniumProtocol fromDesiredCapabilities() instead of getProtocol

public String getPathConsideringDesiredCapabilities() which might return localPath and ignore this.path

public String getPath() which returns ONLY the configured (via the enum's constructor) this.path

Fixed. We now work with only the Map since DesiredCapabilities is not available for access from within TestSlot (that's the place wherein getPathConsideringDesiredCapabilities() would be accessed).

mach6 · 2017-02-08T22:09:37Z

java/server/src/org/openqa/grid/internal/BaseRemoteProxy.java

@@ -342,7 +309,7 @@ public TestSession getNewSession(Map<String, Object> requestedCapability) {
      return null;
    }
    // any slot left for the given app ?
-    for (TestSlot testslot : testSlots) {
+    for (TestSlot testslot : getTestSlots()) {


getTotalUsed(), hasCapability() -- other places in this class still enumerate on the private immutable List testSlots. Will that be a problem for your intended change?

It shouldn't be a problem because the only place wherein we would like to leverage the logic of BaseRemoteProxy is when we are iterating the TestSlot list for new session creation. But just to ensure that we are consistent across the board, I have fixed all the places to access the testSlots list only via the getTestSlots() method.

mach6 · 2017-02-08T22:09:58Z

java/server/src/org/openqa/grid/internal/BaseRemoteProxy.java

      }
    }

    this.testSlots = Collections.unmodifiableList(slots);
  }

-  private SeleniumProtocol getProtocol(DesiredCapabilities capability) {
-    String type = (String) capability.getCapability(SELENIUM_PROTOCOL);


unused import left in this class, as a result of this change

mach6 · 2017-02-08T22:10:06Z

java/server/src/org/openqa/grid/internal/BaseRemoteProxy.java

-  }
-
-  private String getPath(DesiredCapabilities capability) {
-    String type = (String) capability.getCapability(PATH);


unused import left in this class, as a result of this change

krmahadevan · 2017-02-12T16:36:33Z

ping @lukeis @mach6 - Gentle reminder :) Please let me know what else is needed from myside before we get this merged.

mach6 · 2017-02-14T02:07:04Z

java/server/src/org/openqa/grid/common/SeleniumProtocol.java

+    }
+  }
+
+  public String getPathConsideringDesiredCapabilities(Map<String, ?> capabilities) {


But it's not considering "desired" capabilities anymore. It's just capabilities so the method name needs to better reflect this. Same with fromDesiredCapablities above.

fixed again.

krmahadevan · 2017-02-15T03:43:30Z

@mach6 - I have added a test that leverages a custom slot via the proxy. Please let me know if there is anything you need from me additionally to facilitate this merge.

mach6 · 2017-02-15T03:57:12Z

@krmahadevan funny. I just pushed this -- mach6@622abc5

Seems we working this in parallel.

Let's combine the salient points from both our updates to this PR. Will you make an attempt at this or should I?

krmahadevan · 2017-02-15T04:04:27Z

@mach6 - I didn't realise that you were working on this as well. I looked at the commit you shared.
I did add a functional test (just as how you added), and I fixed the SeleniumProtocolTest (boneheaded mistake from my side). IMO there's nothing left to combine in both. You can very well proceed with my PR and if it satisfies all the pre-requisites of the review, I think its good to be merged. Please let me know if that makes sense.

mach6 · 2017-02-15T04:11:25Z

@krmahadevan yep. the test approach is basically the same. I did add javadocs to many of the methods, rename the method from newTestSlot to createTestSlot, and remove some unnecessary Before code in BaseRemoteProxyTest.. I can merge these changes later, I reckon. Either way it should be on top of this PR.

krmahadevan · 2017-02-15T04:21:16Z

@mach6 - Thanks for helping with this and getting this merged.

mach6 · 2017-02-15T04:42:11Z

@krmahadevan please rebase this so we can see what happens with the CI result. grid_tests are broken for this PR. I want to ensure this is not a result of the changes in the PR. Upstream grid_tests are not currently broken. Also please squash.

krmahadevan · 2017-02-15T04:44:51Z

@mach6 - Sure will do.Can you please tell me how do I run the grid_tests alone locally ?

mach6 · 2017-02-15T04:47:25Z

simply run the following from a terminal while in the base directory of the project

$ ./go test_grid

The current implementation does not let downstream consumers of the Grid provide a customised way of injecting a TestSlot. This is very useful when one needs to build an On-Demand Grid, wherein there are no real proxies, but a ghost proxy stands for the real proxy and based on new session requests, nodes are spun off. * Fixed this by defining a method for instantiating a TestSlot so that downstream RemoteProxy implementations can choose to provide their own customised TestSlots. * enhanced the SeleniumProtocol enum to house all logic related to forming the protocol and the path from a desired capability into it. * exposed the matches() method from TestSlot. Downstream consumers of TestSlot would need access to alter the logic of matches() method because sometimes TestSlots would blindly need to return true for all calls to matches() invocation [ This is true when one is building a ghost proxy which doesn’t represent any real node, but stands as a proxy for something such as a docker daemon from where the actual sessions would be honoured ] Without this, end-users would be forced to have their customized TestSlots reside in the same package as that of TestSlot (org.openqa.grid.internal)

krmahadevan · 2017-02-15T05:03:59Z

@mach6 - I found the reason why the build is failing. Its due to java.lang.NoClassDefFoundError: Could not initialize class org.openqa.selenium.WebDriverException that is arising out of SeleniumProtocol because my change includes this exception. But I am not able to figure out how do I get BUCK to include this. Any suggestsions on how do I get this included ?

I have rebased this off of master and squashed my commits into one and pushed them as well.

mach6 · 2017-02-15T06:25:45Z

fyi, the test_grid step passed -- https://travis-ci.org/SeleniumHQ/selenium/builds/201752827

krmahadevan · 2017-02-15T06:42:23Z

Oh ok. I still haven't figured out how Buck works..
Does it mean that we are good now and this PR can be merged ?

krmahadevan · 2017-02-15T06:58:40Z

Ok I guess I now know what is the root cause of the test failures on my mac. For some weird reason NetworkInterface.getByName("en0") returns null. Currently there is no null check in WebDriverException. That explains all the other errors that I faced on my machine. Now I would need to figure out why I don't get a valid NetworkInterface for the name en0 on my MAC

The current implementation does not let downstream consumers of the Grid provide a customised way of injecting a TestSlot. This is very useful when one needs to build an On-Demand Grid, wherein there are no real proxies, but a ghost proxy stands for the real proxy and based on new session requests, nodes are spun off. * Fixed this by defining a method for instantiating a TestSlot so that downstream RemoteProxy implementations can choose to provide their own customised TestSlots. * enhanced the SeleniumProtocol enum to house all logic related to forming the protocol and the path from a desired capability into it. * exposed the matches() method from TestSlot. Downstream consumers of TestSlot would need access to alter the logic of matches() method because sometimes TestSlots would blindly need to return true for all calls to matches() invocation [ This is true when one is building a ghost proxy which doesn’t represent any real node, but stands as a proxy for something such as a docker daemon from where the actual sessions would be honoured ] Without this, end-users would be forced to have their customized TestSlots reside in the same package as that of TestSlot (org.openqa.grid.internal)

Also removed unnecessary before logic in BaseRemoteProxyTest

mach6 · 2017-02-15T20:02:25Z

This stands merged. Thanks!

lukeis reviewed Feb 1, 2017

View reviewed changes

mach6 reviewed Feb 8, 2017

View reviewed changes

krmahadevan force-pushed the krmahadevan-facilitate-testslot-customization branch from e9e4fe3 to 23fe9b1 Compare February 9, 2017 03:59

mach6 reviewed Feb 14, 2017

View reviewed changes

krmahadevan force-pushed the krmahadevan-facilitate-testslot-customization branch 2 times, most recently from 8287fa2 to a1bd741 Compare February 15, 2017 03:42

krmahadevan force-pushed the krmahadevan-facilitate-testslot-customization branch from a1bd741 to e6cd6ef Compare February 15, 2017 04:58

mach6 added a commit to mach6/selenium that referenced this pull request Feb 15, 2017

Minor refactor of SeleniumHQ#3431 and add javadocs.

86a5d70

Also removed unnecessary before logic in BaseRemoteProxyTest

mach6 closed this Feb 15, 2017

krmahadevan deleted the krmahadevan-facilitate-testslot-customization branch February 16, 2017 03:38

Allow for customisation of TestSlot #3431

Allow for customisation of TestSlot #3431

Conversation

krmahadevan commented Jan 28, 2017

krmahadevan commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mach6 Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mach6 Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mach6 Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmahadevan commented Feb 12, 2017

mach6 Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmahadevan commented Feb 15, 2017

mach6 commented Feb 15, 2017

krmahadevan commented Feb 15, 2017

mach6 commented Feb 15, 2017

krmahadevan commented Feb 15, 2017

mach6 commented Feb 15, 2017

krmahadevan commented Feb 15, 2017

mach6 commented Feb 15, 2017

krmahadevan commented Feb 15, 2017

mach6 commented Feb 15, 2017

krmahadevan commented Feb 15, 2017

krmahadevan commented Feb 15, 2017

mach6 commented Feb 15, 2017

mach6 Feb 8, 2017 •

edited

Loading

mach6 Feb 8, 2017 •

edited

Loading

mach6 Feb 8, 2017 •

edited

Loading

mach6 Feb 14, 2017 •

edited

Loading