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

Expose default timeouts to public #59

Closed
zhiburt opened this issue Jun 11, 2021 · 7 comments
Closed

Expose default timeouts to public #59

zhiburt opened this issue Jun 11, 2021 · 7 comments

Comments

@zhiburt
Copy link
Contributor

zhiburt commented Jun 11, 2021

Hi @stevepryde what do you thing to make a public constant or exposed method for a default TimeoutConfiguration?

It would be useful to be able to obtain a default one to be able to switch back after a usage of thirtyfour_query and set_implicit_wait_timeout

// Set default timeouts.
let timeout_config = TimeoutConfiguration::new(
Some(Duration::new(60, 0)),
Some(Duration::new(60, 0)),
Some(Duration::new(30, 0)),
);

@zhiburt
Copy link
Contributor Author

zhiburt commented Jun 11, 2021

By the way it would be also helpful to have some clean method after usage of thirtyfour_query to not have a boilerplate code with a remove of ElementPoller from config and setting default timeouts.

What do you think?

@stevepryde
Copy link
Owner

I think TimeoutConfiguration::default() could be used as the default (both internally and also people can use it to infer the defaults later if needed).

As I've said before, the plan with thirtyfour_query was to merge it into thirtyfour and make it a standard feature. It's probably time to make that happen. I'm not sure how to deal with timeouts though. The easiest way is to just set the default implicit wait to 0 and mention it in the docs. This is a breaking change but in my opinion it is a better default because implicit waits are not as convenient (i.e. when verifying that an element doesn't exist). There are also some other changes in the current main branch that are also breaking changes so now is as good a time as any. It probably needs a warning label in the documentation.

@stevepryde
Copy link
Owner

I think I may have misunderstood your later comment. Were you thinking that WebDriverConfig should have a clean() method or similar? or perhaps that method could exist on WebDriver itself?

@stevepryde
Copy link
Owner

I've implemented Default for TimeoutConfiguration. I also added a way to clear the WebDriverConfig, but it requires mutable access to WebDriver which may pose a challenge. I don't see this as an urgent thing since in the majority of cases it's better to end the session and create a new browser instance rather than try to keep one session open and reuse it.

However if we want to allow the configuration to be updated at any time, then I would suggest putting the entire WebDriverSession inside a mutex i.e. Arc<Mutex<WebDriverSession>> . That should work ok I think. Actually a RwLock may be better since config writes will be quite rare.

@zhiburt
Copy link
Contributor Author

zhiburt commented Jun 12, 2021

I think I may have misunderstood your later comment.

I was just thinking that things we need to set up to use a thirtyfour_query and to switch to default behavior could be hidden by some functions.

But never mind 😉 don't worry to much about it

@stevepryde
Copy link
Owner

stevepryde commented Jun 13, 2021

Regarding removing boilerplate from the thirtyfour_query setup, I've now merged thirtyfour_query into the main branch, so it's available out-of-the-box without any setup. The default implicit wait time is now 0 and the default poller is ElementPoller::TimeoutWithInterval(Duration::from_secs(20), Duration::from_millis(500)) (using ElementPoller::default()). You can set a different poller via WebDriver::set_query_poller()

This means you can now use WebDriver::query(), WebElement::query() and WebElement::wait_until() without any boilerplate setup and it will just work. The traits are also part of the prelude so you don't need to use them specifically.

I suspect the existence of find_element() etc alongside query() may confuse some people though. I'm not really sure how to address that. I may need to make a distinction between lower vs higher level abstractions within WebDriver/WebElement. My preference is to steer people towards the higher-level abstractions but still allow access to the lower-level ones if needed and for those who know what they're doing.

I haven't published v0.25 yet.

@zhiburt
Copy link
Contributor Author

zhiburt commented Jun 13, 2021

looking forward 0.25 :)

@zhiburt zhiburt closed this as completed Jun 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants