-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Allow for customisation of TestSlot #3431
Allow for customisation of TestSlot #3431
Conversation
ping @lukeis - Can you please help vet this out ? |
private void computePath(DesiredCapabilities capability) { | ||
String localPath = (String) capability.getCapability(PATH); | ||
if (localPath != null) { | ||
this.path = localPath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukeis - Thanks for catching this. I completely missed that part. I will fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukeis - I have fixed this and also added a unit test as well.
* @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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a test that exercises the changes to TestSlot
, overriding the default behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @mach6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you be able to create a DummyTestSlot
for the purposes of a functional test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)
} | ||
} | ||
|
||
public String getPath(Map<String, Object> capabilities) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofgetProtocol
public String getPathConsideringDesiredCapabilities()
which might returnlocalPath
and ignorethis.path
public String getPath()
which returns ONLY the configured (via the enum's constructor)this.path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getTotalUsed(), hasCapability() -- other places in this class still enumerate on the private immutable List testSlots
. Will that be a problem for your intended change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
} | ||
|
||
this.testSlots = Collections.unmodifiableList(slots); | ||
} | ||
|
||
private SeleniumProtocol getProtocol(DesiredCapabilities capability) { | ||
String type = (String) capability.getCapability(SELENIUM_PROTOCOL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused import left in this class, as a result of this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
} | ||
|
||
private String getPath(DesiredCapabilities capability) { | ||
String type = (String) capability.getCapability(PATH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused import left in this class, as a result of this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
e9e4fe3
to
23fe9b1
Compare
} | ||
} | ||
|
||
public String getPathConsideringDesiredCapabilities(Map<String, ?> capabilities) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed again.
8287fa2
to
a1bd741
Compare
@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. |
@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? |
@mach6 - I didn't realise that you were working on this as well. I looked at the commit you shared. |
@krmahadevan yep. the test approach is basically the same. I did add javadocs to many of the methods, rename the method from |
@mach6 - Thanks for helping with this and getting this merged. |
@krmahadevan please rebase this so we can see what happens with the CI result. |
@mach6 - Sure will do.Can you please tell me how do I run the |
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)
a1bd741
to
e6cd6ef
Compare
@mach6 - I found the reason why the build is failing. Its due to I have rebased this off of master and squashed my commits into one and pushed them as well. |
fyi, the |
Oh ok. I still haven't figured out how Buck works.. |
Ok I guess I now know what is the root cause of the test failures on my mac. For some weird reason |
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
This stands merged. Thanks! |
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.
X
in the preceding checkbox, I verify that I have signed the Contributor License Agreement