SeleniumLibrary icon indicating copy to clipboard operation
SeleniumLibrary copied to clipboard

Review create webdriver code and tests

Open emanlove opened this issue 3 years ago • 6 comments

With the recent removal of the Opera references I noticed some discrepancies and some things that just didn't look right within this code. I want to review that code now there are some significant changes. This is a placeholder reminder.

emanlove avatar Oct 05 '22 10:10 emanlove

Also review for phantomJS, Andriod, and iPhone references as well

emanlove avatar Oct 05 '22 10:10 emanlove

Another item we should check is the parameters for creating webdriver. I see an additional parameter service on the Chrome and Chromium WebDriver classes that I am not sure we are either describing (very likely) or passing along (I think unlikely).

emanlove avatar Oct 23 '22 12:10 emanlove

Hi Ed, I also notice that creating a webdriver does not work as expected (Selenium 4.7.2, SeleniumLibrary 6.0.0).

# (The browser type does not really matter)
Create Webdriver  chromium

calls browsermanagement > BrowserManagementKeywords > create_webdriver

https://github.com/robotframework/SeleniumLibrary/blob/24c732b1628420ab8fd016424489ba1d63667503/src/SeleniumLibrary/keywords/browsermanagement.py#L383-L390

creation_func should return the browser specific webdriver class (instantiated in line 391), but all it returns is the module <module 'selenium.webdriver.chromium'>:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: 'module' object is not callable

(its __init__.py is empty, no code runs)

However, while debugging, when I am assigning the Chromium Webdriver class by hand, creation_func starts the browser:

# (halted in line 391) 
from selenium.webdriver.chrome.webdriver import WebDriver
creation_func = WebDriver
creation_func(**init_kwargs)
# (chromium starts successfully)
<selenium.webdriver.chrome.webdriver.WebDriver (session="62a68ae2a4b6687ae0f76df8cc45883e")>

I am far from being able to judge whether my approach is correct or to decide what exactly is the error. But yes, there is something broken (but you already knew that before ;-) )

simonmeggle avatar Dec 21 '22 16:12 simonmeggle

I wasn't aware of an issue this in depth. I just know that some recent changes in both Selenium and SeleniumLibrary around supported browsers and the underlying Open Browser keyword/functionality threw up red flags in my mind. So I wanted a full review. Thanks for this report!

emanlove avatar Dec 21 '22 17:12 emanlove

OMG. Wrong path... 🤦‍♂️ It makes a big difference how you write the browser name. https://github.com/robotframework/SeleniumLibrary/blob/24c732b1628420ab8fd016424489ba1d63667503/src/SeleniumLibrary/keywords/browsermanagement.py#L385

In case of "Chrome" it returns the webdriver object. But "chrome" is also an attribute of this module and returns the submodule (totally wrong).

getattr(webdriver, "chrome")
<module 'selenium.webdriver.chrome' from 'C:\\Program Files\\Python38\\lib\\site-packages\\selenium\\webdriver\\chrome\\__init__.py'>
getattr(webdriver, "Chrome")
<class 'selenium.webdriver.chrome.webdriver.WebDriver'>

Maybe the first letter ofdriver_name should be forced to a capital letter before reading the attrs, so that "chrome" is also a valid argument?

(EDIT: Sorry, has nothing to do with the actual problem now. )

simonmeggle avatar Dec 21 '22 17:12 simonmeggle

That looks like a usability bug to me. So I think it still counts!

One of those robot improvements that's on my list would be situations like these where either a warning is given if it looks like someone has an invalid argument name. For example, Log console_msg=ed_was_here might result in Warning: Did you mean to use "console" argument instead of "console_msg" .. or something similar. There could also be the case you share where it is very close.

emanlove avatar Dec 21 '22 18:12 emanlove