Selenium options string to object #1393
Conversation
|
Python 3.4 has reached EOL so there's no real need to support it anymore. Python 3.5 doesn't preserve dictionary order either, though. Need to wait until Python 3.6 for that. |
|
Good point and that makes me think should SL drop support for Python 3.4? Or will the announcement come too late and perhaps it should be postponed to next major release? |
|
Don't think it matters much either way. It's unlikely there are many users that would be affected, but I doubt there's any extra work supporting Python 3.4 either. If the next release is close, perhaps support 3.4 with it still but then drop the support. I guess that would mean only stopping testing with it and removing classifier for |
|
It seems that ChromeDriver has an issue with Selenium ActionChains and not all works like in the previous version. Hard coded the ChromeDriver version for now in #1394 |
|
Thank you for the comments, useful feedback. I did provide answers to the points which needed clarification. If I did not comment, I do agree with the comment. |
|
Changed the options string format. All the other comments are not yet done. |
|
I was thinking, that if there is this argument which has semicolon inside, example like this |
|
I actually commented about handling semicolons that are part of arguments earlier. They can be handled by tokenizing the string and, assuming the string is Python, that can be done using tokenize.generate_tokens. It has a really strange API, but BuiltIn.evaluate uses it succesfully. Here's a prototype: >>> from tokenize import generate_tokens
>>> import token
>>> from StringIO import StringIO # Python 2
>>> options = "method('arg;with;colons'); attr = 'value;with;colons'"
>>> for toknum, tokval, start, _, _ in generate_tokens(StringIO(options).readline):
... if tokval == ';' and toknum == token.OP: # checking toknum may not be needed
... row, col = start # should we take into account multiline strings?
... print("Found '%s' at index '%s'." % (tokval, col))
...
Found ';' at index '25'.
>>> print(options[:25])
method('arg;with;colons')
>>> print(options[25+1:].lstrip())
attr = 'value;with;colons'The same approach could possibly be used to separate |
|
I did not see that it would be possible in that way. And true, it has really strange API. Will fix the code. |
|
I'd create a helper function/method that accepts the option string, finds semicolons, and yields individual options. Last item returned by generated_tokens is the logical line containing the token and the part before the |
|
Improved the parser to only split between methods or attributes. Also fixed the documentation, but other comments are not yet fixed. |
|
I did read the review comments all should be now fixed. Although reading documentation would be helpful. |
|
One this is merged, I was planning to do new alpha release. Then fix last few opens issues and push release candidate out. |
| Selenium options. Example for Chrome, the ``options`` argument | ||
| allows defining the following | ||
| [https://seleniumhq.github.io/selenium/docs/api/py/webdriver_chrome/selenium.webdriver.chrome.options.html#selenium.webdriver.chrome.options.Options|methods and attributes] | ||
| and for Firefox these |
There was a problem hiding this comment.
Just checking to make sure Firefox profile does not overlap or interfere with Firefox options ..? Read through the API docs and it looks like no they are mutually exclusive.
There was a problem hiding this comment.
Those two partially overlap but generally Firefox options allows to do more. But they are not exclusive, one can define both and all should work fine. I did briefly tested the scenario and did not find problems. Although I did not dive deeply on the matter and I don't know if same setting is defined in both, which one has priority over other.
| `add_argument("--headless")`. But `add_argument(" --headless ")` is | ||
| not same same as `add_argument ( "--headless" )`, because | ||
| spaces inside of quotes are not removed. | ||
|
|
There was a problem hiding this comment.
One difficulty with supporting a string formatted options is being backwards compatible over time or change the rules of how the string is formatted. You might try out the formatting with several users to see how it works for them.
There was a problem hiding this comment.
That is a worry which I also have, but there are some safe guards. Calling the Selenium options is as much as possible backwards compatible, because behind the scenes it does a similar trick as the Create WebDriver keyword with Python getattr, setattr and callable, more details in here. Therefore keyword leaves to the user to correctly defined and maintain the methods and attributes user wants to call.
But there is a part that worries me and that is the importing options. First Selenium options import path may change or the options class name may change. In the past, that part has been quite stable and I have not seen any changes in the upcoming Selenium 4.0 release either. But who knows what future will bring.
Also mobile devices, which are supported on the Open Browser are always done by using the webdriver.Remote. The remote WebDriver supports options object, but those mobile devices don't have their own opinions object (Like example Chrome or Firefox has). Therefore the keyword does not support creating options object for those mobile devices by using the string format. Also there is feature/bug when Selenium options instance object is passed directly in, for mobile devices, it will fail, but that feature can be lifted away. But there is some indication that for example Android might directly support Chrome options http://chromedriver.chromium.org/getting-started/getting-started---android Also I know that some users are using SeleniumLibrary for mobile testing because, according to some users feedback: "SeleniumLibrary has better keywords but is little bit tricky to setup." Therefore it might be interesting to support also options for mobile devices, but currently I lack the knowledge how that part actually works and the Travis testing infrastructure is not not setup (Although there seems to be support for mobile)
In any case, if the limitation in the string format is left as it is, it should be documented. Also we should remove the limitation when the Selenium options instance is passed in for mobile devices. That limitation should be easy to change, because now we support only two formats.
There was a problem hiding this comment.
It's impossible to make this fully future compatible because Selenium doesn't have a stable API for specifying options. The current approach to support specifying options like method('arg') and attr = 'value' has a benefit that users can adjust their code if Selenium changes.
There was a problem hiding this comment.
That is true. I still need to think should we do some extra for the mobile users. But perhaps easiest way is to:
- Fix the bug with mobile and Selenium options
- Android == Chrome for selenium options. + Need documentation.
Make alpha release and ask specially for people using mobile to take a look.
|
Android == Chrome options but is not documented. Once that is done, this monster can be merged to the master. |
Fixes #1331