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

Default values of options for withNewWindow() and withWindow() should be configurable #406

Closed
mattbroekhuis opened this Issue Oct 1, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@mattbroekhuis

mattbroekhuis commented Oct 1, 2015

I'm a bit new to geb, so I might be missing something here... But we have a large testing codebase working with a site that relies heavily on new window popups. Most of the stuff looks like this

 withNewWindow({ countryPopup.click() }, page: CountryQuery, close: true) {
            at CountryQuery
            CountryQuery.with{
                //do stuff
            }
        }

Works fine on local dev laptops, but when we run on our selenium grid we get intermittent failures. My guess is that system load is high and response times are slow. Is there a way to set the default wait time for a new window to come up across the board? The documentation and reading source indicate that you have to explicitly set it on every single "withNewWindow" call.

@arvidj01

This comment has been minimized.

arvidj01 commented Oct 1, 2015

Matt and I (we work together) have reviewed this and believe part of the confusion is over the parameter name 'wait'.

There was an expectation that this 'wait' was the same 'wait' as the template 'wait' ... spelled the same and the documentation says "The possible values for the wait option are consistent with the ones for wait option of content definitions." so we assumed that the "default values" under the wait options definition also held for the withNewWindow wait.

Anyway, the withNewWindow 'wait' is how long to 'wait' for the number of windows to increase by 1, which, thought involving time, is a different concept than what is associated with the other 'wait'. And it does not appear to respect the "default wait" definition as there is no 'waiting' ... not even the default ... if the option is not specified on the method.

If the window list increase goes well then, if a page was specified, the 'at' processing for the page ... and any associated 'wait' on the template in that process ... is performed.

Now that we know which wait is which we can get past our immediate issues but will suggest that there be a configuration setting for the 'new window wait' the same as there is a configuration setting for the 'template wait'.

Thanks,
Arvid

@erdi

This comment has been minimized.

Member

erdi commented Oct 4, 2015

@arvidj01 wrt to the naming, it might be confusing but the option is pretty well documented:

You can specify wait option if the action defined in the window opening closure passed as the first argument is asynchronous and you need to wait for the new window to be opened.

The bit that follows and which you quoted is about accepted values for the option and not about how they're used. I believe that it is clear. Also the same name gives us conciseness which means that you have don't to type too much, especially given that it's not possible to get autocompletion for these options in Intellij. There is no waiting occurring by default for withNewWindow() because as per documentation the default option for this option is null which means no waiting.

I agree that it will be useful to be able to configure what are the default values for the options of withNewWindow() if you don't specify any. We already have #373 which is about being able to configure defaults for template options so this would be a logical addition. I will modify this issue to be about configuring default option values for withWindow() and withNewWindow().

Just a side note, I'm not sure if you are aware but configuring default waiting doesn't mean that each of your content definitions will be waited for - it only tells Geb how long to wait for if you pass true as the value of the wait option to content definition or a method that takes a wait option, e.g. withNewWindow(). If it's not clear from the docs then I'm happy to accept a PR with improvements.

@erdi erdi changed the title from Default wait timeout for withNewWindow() to Default values of options for withNewWindow() and withWindow() should be configurable Oct 4, 2015

@erdi

This comment has been minimized.

Member

erdi commented Oct 4, 2015

The configurable options should be:

  • wait, close for withNewWindow()
  • close for withWindow()

This issue is related to #373.

@erdi erdi added this to the 1.0 milestone Oct 4, 2015

@arvidj01

This comment has been minimized.

arvidj01 commented Oct 4, 2015

Ah, I see where our confusion comes from.

All of our pages have a "waitFor" on the 'at'. "waitFor" can have parameters but "to specify nothing is to use the default".

When we started using withNewWindow there was an assumption made that "to specify nothing on a wait" would have the same effect ... as in 'use the default". Careful reading of the documentation proves that assumption to be incorrect ... as in, for a 'wait', 'to specify nothing is to do nothing'

Sorry about creating an issue where it was our failure to understand the documentation.

For #373, is the intent of the new default options be to follow "to specify nothing is to use the default" or "to specify nothing is to do nothing" on withNewWindow"?

If it is the latter than we would still need to use 'wait: true' on all of our 'withNewWindow'. Given that requirement we can simplify everyone's life by creating our own configurable attribute and use "wait: windowWait" on the withNewWindow. From our perspective the effect is the same and it saves you from having to create a configurable 'windowWait' [#373] which would only be effective if we use 'wait: true' anyway.

Thanks,
Arvid

@erdi

This comment has been minimized.

Member

erdi commented Oct 4, 2015

I'm not sure what you mean in the last 2 paragraphs to be honest.

Basically, after #373 and this issue are implemented you will be able to say what the default is for specific options - by default I mean situation when you're not passing any value for a given option. So you will be able to say things like:

  • if I don't specify a value for the wait option when calling newWithWindow() then please use the default waiting time (and the default waiting time can also be configured)
  • if I don't specify a value for the wait option on a content definition then please use the waiting time as configured for the slow preset
  • ...

Did I explain it clear enough?

@mattbroekhuis

This comment has been minimized.

mattbroekhuis commented Oct 4, 2015

Sounds great.

On Oct 4, 2015, at 9:35 AM, Marcin Erdmann notifications@github.com wrote:

I'm not sure what you mean in the last 2 paragraphs to be honest.

Basically, after #373 and this issue are implemented you will be able to say what the default is for specific options - by default I mean situation when you're not passing any value for a given option. So you will be able to say things like:

if I don't specify a value for the wait option when calling newWithWindow() then please use the default waiting time (and the default waiting time can also be configured)
if I don't specify a value for the wait option on a content definition then please use the waiting time as configured for the slow preset
...
Did I explain it clear enough?


Reply to this email directly or view it on GitHub.

@arvidj01

This comment has been minimized.

arvidj01 commented Oct 5, 2015

Erdi,

That is what I was looking for ... "please use the default waiting time" ... without having to provide some value [like 'wait: true] on every withNewWindow() call.

Thanks,
Arvid

@erdi erdi removed this from the 1.0 milestone Dec 29, 2015

erdi added a commit to geb/geb that referenced this issue Jun 18, 2018

erdi added a commit to geb/geb that referenced this issue Jun 18, 2018

@erdi erdi self-assigned this Jun 18, 2018

@erdi erdi added the New feature label Jun 18, 2018

@erdi erdi added this to the 2.2 milestone Jun 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment