@timeout annotation is forcing implicit waits onto users

andrew-sumner commented 8 years ago

As issue #108 (and PR #110) has been closed prematurely and I cannot reopen it, I've started a new issue.

@Ianwen Regardless of how long its been in use, the currently implementation alters the default behaviour of selenium and I consider it to be a bug.

You were right to reject my PR but you've done it for the wrong reasons. I was a bit hasty with my original PR as my implementation would likely have overridden the timeout set by driver.manage().timeouts().implicitlyWait(10, TimeUnit.SECONDS); and made it zero - which is just as bad as the currently implementation which I believe forces it to 5.

The behaviour I would like to see is:

the @timeout annotation should be used to introduce custom wait times for elements, overriding the default behaviour set by driver.manage().timeouts().implicitlyWait(10, TimeUnit.SECONDS); on that particular element - currently @timeout overrides this setting globally even if like me you don't use it
the system property webdriver.timeouts.implicitlywait should be removed - it was a poor design choice.

If you still don't want to change the @timeout implementation then I'd like to see the opinions of a few other project members on this issue rather than just yours as I believe this issue is rather important.

Yes, its easy to work around as you highlighted when closing the previous issue, but in most cases:

users won't be aware of the @timeout feature as it is poorly documented
users won't be expecting HtmlElements to alter the fundamental behaviour of selenium
users wanting to do implicit waits would set driver.manage().timeouts().implicitlyWait(10, TimeUnit.SECONDS); but the current implementation of @timeout completely overrides that behaviour
the selenium project advises against mixing implicit and explicit waits (see http://www.seleniumhq.org/docs/04_webdriver_advanced.jsp) and yet you are forcing this behaviour onto users who might not be aware of the @timeout annotation and it's implications
detecting the issue is a matter of luck - I only discovered it when debugging through my application and noticed that it was taking an unusually long time to fail when an element wasn't available

If you are not willing to improve the timeout behaviour then at the very least the projects readme must be given a section devoted to the @timeout annotation and its flaws.

lanwen commented 8 years ago

summon @artkoshelev

andrew-sumner commented 8 years ago

I've done a little more investigation since posting this and unfortunately not had the time to take it further. I at least understand why the @timeout feature has been built this way - selenium unfortunately don't provide a way to get at the implicit wait setting so you've been forced to implement a complete replacement for selenium's implicit timeout setting.

This feature is a great idea, selenium's implicit wait implementation somewhat resembles a sledgehammer and is damn near impossible to customise. I still have two issues with the current implementation though:

it introduces unexpected behaviour: anyone who has been using selenium for a while does not expect an implicit wait unless they specify one, the current implementations default 5 second implicit wait therefore comes as a bit of an unwelcome shock when discovered
the timeout feature is not documented in a obvious place like the readme which is important given the behaviour it introduces

So in short, I still think the current @timeout implementation could be improved:

the implicit timeout period should default to zero to follow the same conventions as selenium. Yes this change would affect existing users but from what I can tell when the @timeout feature was implemented it came with a 5 second implicit timeout without informing anyone and with proper documentation it shouldn't be much of a issue
a "nicer" method for setting the timeout period would be good, setting the system property feels a bit hacky
the functionality should be highlighted in the readme - which might get it some more users as well!

artkoshelev commented 8 years ago

@andrew-sumner thank you for investigating the problem

not agreed, zero seconds timeout is as unexpected as 5 seconds, we should probably get it from driver
agredd, using this system property comes from older times and only grand-fathers of automation can remember using it in selenium (and it's not documented)
completely agreed here, PR's are welcome, you know :)

andrew-sumner commented 8 years ago

I have to disagree with you about 0 second wait being as unexpected as a 5 second wait:

From the selenium documentation: http://www.seleniumhq.org/docs/04_webdriver_advanced.jsp

Implicit Waits An implicit wait is to tell WebDriver to poll the DOM for a certain amount of time when trying to find an element or elements if they are not immediately available. The default setting is 0. Once set, the implicit wait is set for the life of the WebDriver object instance.

and also

WARNING: Do not mix implicit and explicit waits. Doing so can cause unpredictable wait times. For example setting an implicit wait of 10s and an explicit wait of 15 seconds, could cause a timeout to occur after 20 seconds.

Clearly anyone who is using implicit waits expects that the default would be zero. I know that yours is an alternative implementation but surely it's best to conform to existing behaviour where possible?

I've also looked into the history of this feature and 9 months ago the HtmlElementLocatorFactory was changed from:

return new AjaxElementLocator(searchContext, new HtmlElementFieldAnnotationsHandler(field));

to:

return new AjaxElementLocator(searchContext, getTimeOut(field), new HtmlElementFieldAnnotationsHandler(field));

which I would argue is the breaking change and has altered the expected behaviour for anyone using this project.

I'm happy to provide a PR to address documentation and a better way of setting default timeout, but I really would like to get some agreement that the default timeout should be zero...

Wait Strategy Tests

I've run some tests as I was curious to see what would happen with different wait strategies, source code is at https://github.com/andrew-sumner/htmlelements/tree/timeout/htmlelements-java/src/test/java/andrew

System.setProperty("webdriver.timeouts.implicitlywait", "0");

Test	Failed In	Expected
No Wait: Not Annotated	0	0
No Wait: @Timeout(5)	5	5
Implicit Wait (3): Not Annotated	6	3 - not sure what going on here
Implicit Wait (3): @Timeout(5)	12	unknown - not advised to mix
Explicit Wait (3): Not Annotated	3	3
Explicit Wait (3): @Timeout(5)	3	5 - seems to override @timeout
Fluent Wait (3): Not Annotated	3	3
Fluent Wait (3): @Timeout(5)	3	5 - seems to override @timeout

System.setProperty("webdriver.timeouts.implicitlywait", "5");

Test	Failed In	Expected
No Wait: Not Annotated	5	I'd expect 0 but current implementation expects 5
No Wait: @Timeout(5)	5	5
Implicit Wait (3): Not Annotated	12	unknown - not advised to mix
Implicit Wait (3): @Timeout(5)	12	unknown - not advised to mix
Explicit Wait (3): Not Annotated	5	3
Explicit Wait (3): @Timeout(5)	5	unknown
Fluent Wait (3): Not Annotated	5	3
Fluent Wait (3): @Timeout(5)	5	unknown

System.setProperty("webdriver.timeouts.implicitlywait", "5");

Test	Failed In	Expected
No Wait: Not Annotated	5	I'd expect 0 but current implementation expects 5
No Wait: @Timeout(5)	5	5
Implicit Wait (7): Not Annotated	21	unknown - not advised to mix
Implicit Wait (7): @Timeout(5)	21	unknown - not advised to mix
Explicit Wait (7): Not Annotated	10	7
Explicit Wait (7): @Timeout(5)	10	unknown
Fluent Wait (7): Not Annotated	11	7
Fluent Wait (7): @Timeout(5)	11	unknown

artkoshelev commented 8 years ago

If you dig a little deeper, you'll find this commit existing almost from the very beginning of project. Using default implicitly wait will save you a lot of code and nerves. Mixing it with explicit waits is not unpredictable, but has expected limits, so will not harm. In rare case of testing specific timeout on element you can annotate it with @Timeout(0).

andrew-sumner commented 8 years ago

I give up, close this if you want. I'll be creating a custom locator factory for use on my projects that default to a 0 second wait.

I get that you guys like implicit waits, but there is a large community out there that do not and this project as it stands doesn't cater well to us.

I personally find that I only need to wait on one or two elements on a page, the majority of the time waits (implicit or otherwise) just aren't required so even if I do use the timeout annotation I will still set the default to 0.

I can't recommend enough that you add some documentation to highlight this behaviour.

IZaiarnyi commented 8 years ago

Do we have any ability to set default timeout to 0 instead of 5 seconds without setting @Timeout ?

artkoshelev commented 8 years ago

@IZaiarnyi please ask your questions on stackoverflow

lehvolk commented 7 years ago

+1 for removing implicit waits or sync them with driver. Just investigated performance problems with our autotests and figure out 2 problems:

SlowLoadingElementList load list for 5 seconds if target list is empty. If UI for example contains of embedded almost all empty tables then iteration for low levels cells can be dramatically slow.
if you want to check existence of any element (modal window for example) you will wait for 5 seconds if it is closed.

We had a special implicitWait setting for driver but as I figure out it won't help. VM option webdriver.timeouts.implicitlywait setted to 1 just make our tests 4 time faster.

@timeout annotation and implicitWaits is hardly discoverable. Only digging source code and profiling tests bringing light on that problem.

Zlaman commented 6 years ago

There is one more issue related to this. If I want to disable implicit waits completely via

System.setProperty("webdriver.timeouts.implicitlywait", "0");

I am getting exception in case I want to interact with any of WebElement initialized with HtmlElementLocatorFactory

Exception in thread "main" java.lang.NullPointerException
    at ru.yandex.qatools.htmlelements.loader.decorator.proxyhandlers.WebElementNamedProxyHandler.invoke(WebElementNamedProxyHandler.java:50)

If I set timeout to 1 second, everything works good. So currently it is not possible to switch off implicit wait when using HtmlElementLocatorFactory to initialize PageFactory

artkoshelev commented 6 years ago

@Zlaman had you tried the latest snapshot version? The latest commit in master fixes the issue you described: https://github.com/yandex-qatools/htmlelements/commit/736f3faf263e140b6c789eccdcf34e07e533e86c

Zlaman commented 6 years ago

@artkoshelev no, I am using release 1.18 version from maven. How to take latest snapshot version from maven?

artkoshelev commented 6 years ago

@Zlaman just change version to 1.19-SNAPSHOT

Zlaman commented 6 years ago

Unfortunately it does not work. Looks like maven central cannot store snapshot versions, only releases. http://search.maven.org/#search%7Cgav%7C1%7Cg%3A%22ru.yandex.qatools.htmlelements%22%20AND%20a%3A%22htmlelements-java%22

Example of pom.xml

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://maven.apache.org/POM/4.0.0"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>

    <groupId>test</groupId>
    <artifactId>test</artifactId>
    <version>1.0-SNAPSHOT</version>

    <dependencies>
        <dependency>
            <groupId>ru.yandex.qatools.htmlelements</groupId>
            <artifactId>htmlelements</artifactId>
            <version>1.19-SNAPSHOT</version>
        </dependency>
    </dependencies>
</project>

andrew-sumner commented 6 years ago

We've been using JitPack use the lastest version.

yandex-qatools / htmlelements