Explicit

[titusfortner]

Change list

• Changes name of constant since it actually isn't being used implicitly
• Removes the resetting of implicit waits

Types of changes

What types of changes are you proposing/introducing to Java client?
Put an x in the boxes that apply

• No changes in production code.
• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• [x ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

Implicit waits are currently entirely overridden. Setting implicit wait to 0.5 or 300 won't make any difference when locating elements.

I don't think we should remove the user's ability to set an implicit wait, even if it can have negative consequences for them.

This is a "breaking" change because implicit waits will now be respected, which can allow users to combine implicit and explicit in ways that are not ideal, or cause longer test runs if users are testing for the absence of an element. But I think them not being respected before was technically the unexpected behavior, and this is essentially a bug fix for that.

I think this should be released with the recognition that people might have timing issues and that they should be told not to set implicit waits if they want to keep previous behavior.

I'm trying to think about how overriding the implicit wait setting in the appium driver, storing the value and doing fancy things with it during location method could work... but all I come up with would remove options from the user that I think they should have.

Anyway, let's discuss. Feel free to use/modify this code as necessary in anything you end up doing..

[titusfortner]

Oh, forgot to mention this is for #735

[TikhomirovSergey]

@titusfortner Thank you very much. I will review it soon.
I think we should warn users to not use driver..manage().timeouts().implicitlyWait() if they use page/screen objects.

[mykola-mokhnach]

-> timeout

[titusfortner]

There are 32 places that use timeOut vs none with timeout. This PR should keep it consistent, though a later commit to change all usages is probably appropriate.

[SrinivasanTarget]

Why can't we change all the instances?

[titusfortner]

We can/should, just didn't want to deal with those changes in this PR/release. :-)

[TikhomirovSergey]

@titusfortner
It looks ok. But could you remove the changeImplicitlyWaitTimeOut? It seems to be unused.

[titusfortner]

I think we should warn users to not use driver.manage().timeouts().implicitlyWait() if they use page/screen objects.

I'm not sure what difference it makes page objects vs non. Isn't this locator code what is used regardless?

That being said, I'm all about warning users against using implicit waits, is there a mechanism to do so, or are we just talking about in the responses we give in the likely github issues? :)

@TikhomirovSergey - I removed changeImplicitlyWaitTimeOut, so I think this should be good.

[SrinivasanTarget]

That being said, I'm all about warning users against using implicit waits, is there a mechanism to do so, or are we just talking about in the responses we give in the likely github issues? :)

@titusfortner @TikhomirovSergey AFAIK Selenium is planning to deprecate implicit waits soon. Simon had spoke about deprecating it on last SeleniumConf. May be we can wait for it.

[TikhomirovSergey]

@SrinivasanTarget ok. So there is nothing to worry about.

[titusfortner]

@SrinivasanTarget Simon said not to use them, but he still put them in the w3c spec, so I don't think they'll be actually deprecated any time soon.

[SrinivasanTarget]

Simon said not to use them, but he still put them in the w3c spec, so I don't think they'll be actually deprecated any time soon.

Ok