Skip to content
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

Add API to set page load timeout #1535

Closed
0xLeon opened this issue Jan 8, 2020 · 4 comments
Closed

Add API to set page load timeout #1535

0xLeon opened this issue Jan 8, 2020 · 4 comments
Labels
acknowledge To be acknowledged in release notes enhancement good first issue Good for newcomers priority: high
Milestone

Comments

@0xLeon
Copy link

0xLeon commented Jan 8, 2020

Today, I stumbled upon the lack of ability to set the page load timeout defined by the W3C WebDriver standard [1]. The underlying Python Selenium implementation does provide a corresponding set_page_load_timeout method in selenium.webdriver.remote.webdriver.WebDriver [2].

Too me, it is not completely clear how this should be handled on the library level. SeleniumLibrary currently has two kinds of timeouts. The library timeout is both the library-own default timeout for the waiting keywords as well as the script timeout from the WebDriver standard. The implicit wait timeout is only a direct pass-through to the implicit wait timeout from the WebDriver standard. I don't know exactly why the WebDriver standard has a separate page load timeout, but it should be exposed on the library level as well, I think.

The question is, should SeleniumLibrary just use the existing library-level timeout for the page load timeout as well? This would mean that you basically loose the separation on the WebDriver level and have a single configuration parameter for script timeout and page load timeout.
The other solution would be to add another set of parameters and keywords for the page load timeout itself. This way, the separation from the WebDriver level is also exposed on the library level. But it would probably mean an API-breaking change is necessary, adding another parameter to the library __init__ as well as some more keywords specifically for the page load timeout.

[1] https://w3c.github.io/webdriver/#timeouts
[2] https://github.com/SeleniumHQ/selenium/blob/5d0099de8401c3e5b58f58bd227c46d95da7c8a7/py/selenium/webdriver/remote/webdriver.py#L967

@0xLeon
Copy link
Author

0xLeon commented Jan 8, 2020

One more note: I'd be happy to provide a pull request for either solution, but I can't say for sure when this would be possible due to the necessary code contribution approval process from my employer.

@aaltat
Copy link
Contributor

aaltat commented Jan 8, 2020

Page load timeout is (usually) bigger than the other timeouts in the library. Usually current timeouts are less then 60 seconds (more close to 15 seconds in my usual use) and page load timeout can be multiple of minutes. Of course this highly depends on the web application performance and numerous other factors.

At least we should add keyword to set the page load timeout, in similar functionality Set Selenium Timeout. But should page load timeout be also library level import? It is question which does not have clear answer. Page load timeout is not needed that often than other timeouts, but for consistency reason it would be good to have in the library init. I would say that consistency is good thing in this matter and lets add it in the library init too.

This might hard to test in the acceptance test, but it might be easy to test in the unit test.

@ricardomaximino
Copy link

Hi, @0xLeon
Do you have any work around or possible solution you could share?!

emanlove added a commit that referenced this issue Apr 27, 2023
…e-load-timeout

 Add API to set page load timeout (#1535)
@emanlove emanlove added this to the v6.1.0 milestone Apr 30, 2023
@emanlove emanlove added priority: high acknowledge To be acknowledged in release notes and removed priority: medium labels Apr 30, 2023
@emanlove
Copy link
Member

emanlove commented May 1, 2023

Fixed with #1754

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge To be acknowledged in release notes enhancement good first issue Good for newcomers priority: high
Projects
None yet
Development

No branches or pull requests

4 participants