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

feat: allow to configure implicit wait #147

Merged
merged 8 commits into from
Feb 27, 2025

Conversation

JonasPammer
Copy link
Contributor

@JonasPammer JonasPammer commented Feb 23, 2025

Relates to #134 (comment)

At least for my project, removing it completely results in no errors and 3 minutes (5.0.0-M2; compared to 2m20s without container) instead of 20.

I have yet to figure out a SSCCE that triggers the implicitWait


Sorry for the potential spam of PRs, I want to figure out accepted ways of how to implement things first before tackling my big goal of being able to use GebConfig.groovy (custom environmentized client/driver config + custom remote image) again

Copy link
Contributor

@jdaugherty jdaugherty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution. Let's wait for @matrei to get his thoughts on the config name.

@matrei
Copy link
Contributor

matrei commented Feb 25, 2025

camelCase will be great 👍. That is the standard In Grails.

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JonasPammer! Keep'em coming! I've been away for a couple of days but I'll be more active tomorrow again.

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

I have some more thoughts on this PR:

  • There are two additional timeout values: pageLoadTimeout(default: 300s) and scriptTimeout(default: 30s). I think we should take this opportunity to add support for setting them as well. We could group them under grails.geb.webdriver.timeouts.*. We should also rename the variable and config property to implicitlyWait to align with the name in Webdriver.
  • The default value in Webdriver for implicitlyWait is 0s. I do not know why testcontainers set it to 30s in their examples. I would like to try and set the default to 0s to resolve the issues the 30s default causes in Geb. As I understand it @JonasPammer, your testing shows that 0s works fine?

@matrei matrei changed the title fix: allow to configure implicit wait feat: allow to configure implicit wait Feb 26, 2025
@JonasPammer
Copy link
Contributor Author

i will first make commits that implement it as per #147 (review)

but to be honest, a more natural non-grails-plugin fix would be to allow custom DriverOptions to be passed, as:

An implicit wait value can be set either with the timeouts capability in the browser options, or with a driver method (as shown below). 1

but that will be its own PR when I get to it...

@JonasPammer
Copy link
Contributor Author

good important and oblivious from my side (i saw the defaults too) catches

interesting getInteger method

will do

@JonasPammer JonasPammer requested a review from matrei February 27, 2025 18:35
Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@matrei matrei merged commit 9ddf998 into grails:4.2.x Feb 27, 2025
5 checks passed
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

Successfully merging this pull request may close these issues.

3 participants