Repository navigation
Implement 'Create Webdriver' keyword - #245
Conversation
|
Having a way to configure the underlying Webdriver better is definitely a good idea. Few comments below:
is cleaner and easier to understand than Using **kwargs has two drawbacks, though:
Both of these limitations can be mitigated by accepting |
|
Just wanted to add that I have seen this and I am reviewing it to see what I might be able to add to the conversation. |
|
UPDATE: Selenium team seems to be welcome to having a more "standard" way to create WebDrivers. Thank you so much for you input Pekka. Good software rarely, if ever, comes without peer review. I am also glad you are taking an interest in this project as it is so important to the RF universe. I was coding under that idea that forcing users to upgrade to 2.8 (to use **kwargs) was out of the question. Given your comments and the lack of serious issues found in 2.8, maybe that should be reconsidered. Ed, Jeremy, and Ryan would have to be on board with that. The reasons for not adding the capability to pass arguments to the init's in Open Browser were:
I agree with what you said about normalizing argument names. I just did not want to appear lazy or unwilling to make the code friendlier. I'm not sure how we could accept *varargs and splitting on = would work. If the user is trying to pass in an object, would the object not be converted to a string? If we tried to use BuiltIn.register_run_keyword and parse the arguments inside Selenium2Library, would that not work if Selenium2Library was imported using the WITH NAME syntax and thus had a different name that Selenium2Library would not know about? In summary, I think there is a way to alter Open Browser to handle extra arguments if we can overcome the issue of the argument naming collision and everyone is OK with making 2.8 the minimum version in a upcoming version of Selenium2Library. If we do decide to implement this keyword, it could always be deprecated later thanks to that functionality in RF. As an aside, I realized when writing this keyword that the API docs linked in the keyword documentation did not mention Opera at all. I raised an issue and they fixed it within one hour! That was by Luke, the same person who seems open to having a way to create a WebDriver where all options are in a desired capabilities argument. Thanks again, Pekka! |
|
Hey Guys, @ombre42 thanks for picking this up. I know I said that I would work on it and I've kind of dropped the ball. Anyways as you alluded to in your earlier post the web driver team seems to be open to standardising web driver. I'm working on the patch for selenium now and will hopefully have it out in a few days. The idea is this. You should be able to pass everything into the web driver init function through the desired_capabilities. So I'm creating a function decorator that will decorate all the If that's not too confusing, the code is maybe a bit more confusing :) Well hopefully not. Code is below. I need to clean it up a bit, address a few edge case and most importantly deal with selenium's crazy fun build system. Which is crazy, but certainly isn't fun. :) The other advantage of using the decorator is:
Disadvantage:
Anyways here is a look at the code. I'm thinking if it get accepted, we won't have to make any changes at all to S2L, the existing class AllowDesiredCapabilitesOverrides(object):
'''This is a decorator class intended to decorate the __init__ method for
all webdrivers. It allows the caller to override any argument in the init
method with similarly named key in desired_capabilities. This way
desired_capabilities can be used as a standard way to completely setup
/ configure a webdriver instance.
'''
def __call__(self,f):
'''Stores the various argument names for the function we are
decorating removing the self argument.
Also stores the index of the desired_capabilities argument.
'''
from inspect import getargspec
decorated_func_args = getargspec(f)[0][1:]
caps_arg_index = decorated_func_args.index("desired_capabilities")
def wrap(init_self,*args,**kwargs):
#find desired_capabilities -- thi needs to be expanded as some web drivers use
#different argument names for desired_capabilities
if caps_arg_index < len(args):
caps = args[caps_arg_index]
else:
caps = kwargs.get("desired_capabilities")
if caps:
for count, val in enumerate(decorated_func_args):
if caps.has_key(val):
kwargs[val] = caps.pop(val)
f(init_self,*args,**kwargs)
return wrapDecorating the @AllowDesiredCapabilitesOverrides()
def __init__(self, executable_path="chromedriver", port=0,
chrome_options=None, service_args=None,
desired_capabilities=None, service_log_path=None):What do you guys think? |
|
I think that is a rather elegant solution. I had wondered about how to accomplish what you asked for in issue 6026 while being backwards compatible. One suggestion I have is to use some sort of a prefix for the keys in the DesiredCapabilities dict. @AllowDesiredCapabilitesOverrides(prefix="chrome.init")
def __init__(self, executable_path="chromedriver", port=0,
chrome_options=None, service_args=None,
desired_capabilities=None, service_log_path=None):Overall, it looks like a great solution to the backwards-compatibility issue. |
|
Few fast comments below:
Notice that this is just my opinion. You know this library and its users much better than I do, and I respect your decision either way.
As @ombre42 pointed out, decorators like this loose |
|
Thanks for the feedback. In terms of the doc, and name bit, I haven't tried yet, but I was think I could just reset those in the decorator it self. I don't think it would require the decorator module and I'm trying not to add new module dependencies cause I don't know how the selenium team will respond. I even used monkey patches instead of Anyways @ombre42, I do like the idea of adding an init argument, as I was going to have to try 3 or 4 different argument names to 'find' desired_capabilites. With the init argument I could just create a simple dictionary and key it that way. Although I'm not sure if we also need that argument as a prefix in the desired_capabilities dictionary... is that just to make it clear from the user perspective? With regards to the signature change.... I guess let's ask the Selenium team and see what they say. Anyways I was hopping to get some verification from you guys that this solution will indeed work for our needs. Which I think I'm getting, so I'll go ahead and proceed. Unfortunately I had a bit of a HDD crash (I'm on a friends computer right now). So it may take a couple more days till I get this all wrapped up, but I'll get it out as soon as possible and post it to the selenium forum to get feedback from their side. With any luck they will accept and maybe in a week or so we will have something to work with. |
|
I would agree with @pekkaklarck on not forcing all users to upgrade to RF 2.8. My concern, and the reason for suggesting a second keyword, was to try not being too disruptive with everyone's current tests. I can see a future where one is depreciated and then removed. I'm not completely against change but looking at reducing disruption while improving our long term roadmap. If I am not too late in asking this what is the issue, as suggested here, with using RemoteWebDriver as a single constructor? @j1z0 if I'm not mistaken you said it was the introduction of extra latency for which I would agree this is not desirable. |
|
@emanlove -- Extra latency may be one thing yes, but I think the main reason with using RemoteWebDriver is it doesn't provide for the functionality we are trying to cater for. We are trying to provide a solution to the functionality not provided through desired_capabilities and thus not provided through RemoteWebDriver. So all those init parameters which are driver specific are not available through RemoteWebDriver because all you have in RemoteWebDriver is desired_capabilities. For example if you wanted to load a Chrome extension for testing... can't do it through RemoteWebDriver. There are other examples, but honestly I'm drawing a blank right now. :)... So the point of the desired_capabilites hack it in effect to allow us to treat everything with a standard interface, which would in effect be what using RemoteWebDriver is supposed to accomplish, but we could now do it using the native drivers. |
|
What's odd though is jumping into the code it looks like, looking first at ChromeDriver, those oddball options get converted, using options.to_capabilities() [line 53] for Chrome, and then shoved into desired_capabilities. The others each do it differently (Firefox, IE, Opera, etc.). I must admit I not sure what I am seeing and I am looking very narrow perspective so I trying to deepen my understanding here. @j1z0 your solution looks great. It just looks like a chicken and an egg problem (and I am a confused farmer :) |
|
@j1z0 - Too bad about the HDD crash - I've been there before. There are several reasons for using a prefix, but here is one that makes it an absolute requirement - an existing conflict. There are some changes we need to make to S2L. Currently the capabilities are passed down to _generic_make_browser and then ignored for local webdrivers. |
|
@emanlove - your right about Chrome Options getting converted. As @ombre42 Firefox does something similar with the proxy object. But its not complete. For example you can't really specify a firefox_profile through desired capabilities, You can't even specify the timeout. In chrome you couldn't specify executable_path, service_args (which are flags to pass to chrome_driver) or log path. Really it's just Chrome, Firefox and PhantomJS that are deviant. I agree it is a chicken and an egg thing, and again @ombre42 suggestion to put an init parameter does help clear the air. But now I'm actually starting to question some of my original assumptions. For example trying to pass in a complete JSON representation of a firefox_profile could prove to be a nightmare. Probably better to just pass in the constructor arguments. But then it all seems to start smelling like a giant hack. I don't know.. need to think more.. Brain hurts... |
|
Ok thinking about this some more and just want to bounce around some idea. I looked through the issue list for S2L and here are the issues that are potentially related: Really they break down into four categories.
There is also a request to implement Safari support which should be a no brainer now, that it's there in selenium. Anyways if I try to think of the simplest solution that would work to solve those issues.
The thing that is making me rethink my original solution is how do I know when somebody is passing in firefox profiles or service args? I would have to treat them as special cases, which is actually what they already are in that they are not part of desired_caps. So maybe all we should do is try to standardise the naming for desired_capabilities in selenium and on our side add service_args to the open_browser keyword. I wonder, and please correct me if I'm off base here, if we are trying to solve problems we don't actually have. Is this a case of YANGNI "Your Not Going to Need It".... I really don't know anymore. I'm running around in circle. I'm hopping you guys can set me straight. :) |
|
I’d sure like the Safari support! Thanks, From: Jeremy Johnson [mailto:notifications@github.com] Ok thinking about this some more and just want to bounce around some idea. I looked through the issue list for S2L and here are the issues that are potentially related: Really they break down into four categories.
There is also a request to implement Safari support which should be a no brainer now, that it's there in selenium. Anyways if I try to think of the simplest solution that would work to solve those issues.
The thing that is making me rethink my original solution is how do I know when somebody is passing in firefox profiles or service args? I would have to treat them as special cases, which is actually what they already are in that they are not part of desired_caps. So maybe all we should do is try to standardise the naming for desired_capabilities in selenium and on our side add service_args to the open_browser keyword. I wonder, and please correct me if I'm off base here, if we are trying to solve problems we don't actually have. Is this a case of YANGNI "Your Not Going to Need It".... I really don't know anymore. I'm running around in circle. I'm hopping you guys can set me straight. :) — |
|
or here is another idea. I could stick with the |
|
I think we should just meet at the pub and hash it out :) |
Hear! Hear!
Let's work both fronts. On our side provide a Create Webdriver keyword |
|
Pekka did not respond to my comments. I can certainly add a **kwargs that Here's my comments from issue: |
|
Pub sounds great! Need to start planning conference trips or something similar to your direction. I hope we get this stuff sorted out before that, though. =) I agree with @emanlove's two front approach. Good idea to have some solution now and continue working with a better solution. Sorry @ombre42 for not replying to your comments. What I meant with splitting *varargs from '=' is best illustrated by an example: If you use the above keyword like it will print out the same dict both with RF 2.7 and 2.8. Things get a bit more complicated if the keyword needs normal *varargs, but that's not too hard to handle either. |
|
@ombre42: Rereading your comments I see what we are lacking in response to your question. You want to put in the arguments directly to the webdriver instantation but for Firefox, for example, the proxy argument is an object and not a string. Thus there is no way to pass a proxy object into the Create Webdriver keyword as you have outlined above. I still looking at this but I am formulating that for the 'Create Webdriver' keyword will need to deal with string arguments and build up from there, possibly different for each browser/webdriver, each specific argument; assuming that is, as I am not there yet, we can do these for each argument. For example, for firefox, pass in the proxy argument something like or something similar. I am sorry for not providing a better example as I am fairly busy this week. But I did want to recognize the issue you've brought to light and we have yet to provide a satisfactory answer. I think in the end it is going to be a hack but with some improvements with selenium we should be able to provide a better solution. |
|
I think I need to clarify what the purpose of @pekkaklarck I actually meant to send that comment to Ed, hoping I had missed something. Thank you for responding, and I am sorry I was not more clear. This is an example from my pull - and please ignore that there are ways to send proxy information without using the proxy helper class-this is just an example of some object as an argument: If the caller used the following with named arguments instead, in RF < 2.8 (comes in as varargs not kwargs): In the usual implementation, varargs[0] would be u'proxy=<selenium.webdriver.common.proxy.Proxy object at 0x014B5530>' Pekka, I thought you were suggesting it was possible that we could make varargs in RF < 2.8 work like kwargs does in 2.8 with a little effort. But like I mentioned even if we told RF not to resolve variables before invoking What I am saying is that it does not seem to be feasible to have kwargs-like support via varargs unless all values for arguments are literal strings or repr strings. For I have not tried using register_run_keyword -- just going by the doc. @emanlove - You seem to understand my problem, and thank you for acknowledging that. |
|
@ombre42 Yes, variables containing non-strings work differently depending on do you use The question is, are there people who would be using non-string values that wouldn't be in RF 2.8 already or couldn't easily upgrade? Obviously the same can be asked about whole this new functionality. Perhaps it would be more straightforward to support only **kwargs. Support for RF < 2.8 can be added later if there is enough demand. |
|
Just got another idea. Perhaps the keyword could work like: Now advanced users could create a dictionary containing the capabilities and that would work regardless RF version. For most cases using **extra_capabilities would be more convenient, but obviously require 2.8. |
Documentation also improved and added some missing test coverage
|
@pekkaklarck I thought about having a dict argument instead of |
|
@ombre42 Now that you added support for |
|
I lean towards the idea of using a dictionary. |
acceptance test opens browser passed to run_tests.py
accidentally dropped this coverage in last commit
|
OK Pull request for Selenium is up. Here it is: Please up vote / commit / make sure it's ok and let's try to get it fixed in Selenium. Thanks guys. |
|
Shall I merge @ombre42's changes in? It looks ready to go. I have been working on getting SauceLabs running but need some of these changes to fully utilize their test matrix. |
|
I don't have any objections with the code but should the tests be all passing before it's merged? Cheers,
|
|
@j1z0 Thanks for pointing that out. The tests are still failing for browsers other than Firefox due to issues with the build environment. I am not bothering to process ${DESIRED_CAPABILITIES}. Is that OK? In Chrome, I cannot view the pages served by the test server other than the directory listing. This happens with Chrome outside of the tests (says the page is not responding and asks if I want to kill it). I'm using 31.0.1650.57. Is this a known issue? |
|
hmm seems like my earlier message didn't get posted... weird... anyways basically I would suggest.
As for the chrome issue you are having... don't know what to tell you, it works for me I got same Chrome version and I'm on OSX 10.9. |
Implement 'Create Webdriver' keyword
Please review this commit carefully.
I wish this keyword was not needed, but I do not see the Selenium team moving all options to a desired capabilities argument anytime soon. This keyword will close many issues people have with using a proxy.
A URL parameter is intentionally omitted because an initial get() is not always appropriate (native app testing w/ Appium).
As is, argument names are not checked through reflection and Python will throw an error like:
TypeError: __init__() got an unexpected keyword argument 'bad_arg_name'.Let me know if this is not OK.
possible enancements I can implement if desired:
normalizing argument names and/or WebDriver names and matching via reflection. I did not do this because BuiltIn's Call Method does not do this and this keyword is for advanced users.
possible issues:
The tests will try and launch Firefox even if you call run_tests with another browser.
I tried to test through unit testing, but had problems because the _LoggingKeywords mix-in was not present.