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

Check if element is visible within a timeout without catching an error #2590

Closed
andreisuslov opened this issue Dec 15, 2023 · 6 comments · Fixed by #2640
Closed

Check if element is visible within a timeout without catching an error #2590

andreisuslov opened this issue Dec 15, 2023 · 6 comments · Fixed by #2640
Assignees
Milestone

Comments

@andreisuslov
Copy link

The problem

I propose the implementation of a method in Selenide that allows checking if an element is visible within a specified timeout, without throwing an error if the element does not become visible.
Currently, methods like shouldBe combined with visibility conditions throw an exception if the condition is not met within the specified timeout. While this behavior is suitable for many test scenarios, there are cases where a non-exception-based check would be more appropriate.

Details

A method that returns a boolean value indicating whether an element is visible/enabled/etc within a specified timeout. For instance:

boolean isVisible = $("selector").is(Condition.visible, 10000);

At the moment, something like this could be created in the helper methods locally in my framework:

boolean isVisible;
try {
    $(selector).shouldBe(Condition.visible, Duration.ofSeconds(timeout));
    isVisible = true;
} catch (ElementNotFound e) {
    isVisible = false;
}

, but it appears to me that other developers would also benefit from having this functionality implemented natively in Selenide.

@asolntsev
Copy link
Member

@andreisuslov The question is: what are you going to do with this isVisible variable?

P.S. For background information: https://selenide.org/2019/12/02/advent-calendar-how-to-abuse-selenide/

@andreisuslov
Copy link
Author

Thank you for your work, by the way, it's greatly appreciated. I still would like to address the concerns you raised regarding the proposal for a non-exception-based visibility check in Selenide. The goal here isn't to toss out the whole idea of solid, dependable testing. It's more about flexibility, making things more adaptable and easier to read and use, especially in some specific test cases.

My app is slow. It has dynamic content, and the presence of certain elements may not be guaranteed. The proposed method allows for a more nuanced approach to handle these cases. The tests need to be adjusted for that as the app will likely not get faster. We sometimes need to click onto an element if it's visible, so we wait for it to load. As you pointed out, it can be slow and inefficient but it is what it is - the app under test won't get faster. In my view, this is not about bypassing critical failures but about managing conditional flows within tests.

Imagine a web application that has feature toggles, which might or might not be present depending on the user's settings or A/B testing scenarios.

With the proposed method, you could easily handle this in Groovy as follows:

def checkFeatureToggle() {
    def isFeatureVisible = $("div.toggle").is(Condition.visible, 10000)
    if (isFeatureVisible) {
        // actions related to the feature toggle
        println("feature is available")
        // more code...
    } else {
        // handle the absence of the feature
        println("Feature is not available")
        // alternative code...
    }
}

This code checks for the visibility of a feature toggle within 10 sec. If the element becomes visible within this timeframe, it continues with the related actions. Otherwise, it handles the scenario where the feature is absent.

Currently, we do this:

def checkFeatureToggle() {
    def isFeatureVisible
    try {
        $("div.toggle").shouldBe(Condition.visible, Duration.ofSeconds(10))
        isFeatureVisible = true
    } catch (ElementNotFound e) {
        isFeatureVisible = false
    }
    if (isFeatureVisible) {
        println("Feature is available")
        // more code...
    } else {
        // handle the absence of the feature
        println("Feature is not available")
        // alternative code...
    }
}

Here, you're forced to use a try-catch block to handle the possibility that the element might not become visible. This bulks up your code, making it harder to read. Plus, we are mixing up exception handling with control flow logic. That's like wearing socks with sandals - not exactly stylish.

@BorisOsipov
Copy link
Member

Plus, we are mixing up exception handling with control flow logic. That's like wearing socks with sandals - not exactly stylish.

I always laugh a bit when something like that happens, because if we implement such a waiting underhood, it will be exactly the "try-catch block" you are afraid of. But it is hidden.
That's like wearing socks with sandals but over you wearing women's winter boots to hide your stylish sin🤭

@andreisuslov
Copy link
Author

@BorisOsipov I might not have expressed my point as clearly as i wanted here, my apologies. The main problem I have with this approach is not using a try-catch block, it's that we're coming up with techniques that are wrapping functionality around Selenide, already a wrapper around Selenium on itself. The argument here is to integrate this feature directly into Selenide is based on enhancing usability from the perspective of an end user.

Selenide makes the convoluted mess of Selenium more straightforward. The approaches that we use currently aren't the most elegant, and by integrating the $('element').is(enabled, ofSeconds(10)) functionality directly, we would provide a more intuitive interface.

The issue with the absence of such a feature is that we write half-baked things like this:

   boolean isOkayButtonDisplayed() {
        def okayXpath = "//div[@class='button ok']"
        return $$x(okayXpath).size() == 1
    }

   def doSmth() {
        // some logic
        if (isOkayButtonDisplayed() clickSaveButton()
    }

or

boolean waitUntilDisplayed(SelenideElement element, int timeout = 10000) {
        long endTime = System.currentTimeMillis() + timeout
        while (System.currentTimeMillis() < endTime) {
            if (element.isDispayed()) return element.isDispayed() 
            try {
                sleep(500)
            } catch (InterruptedException e) {
                currentThread().interrupt()
                throw new RuntimeException("Interrupted during wait", e)
            }
        }
    }

This is a (necessary) workaround for the absence of a direct visibility check with a custom timeout. It's more of a workaround than a solution, and integrating the proposed feature into Selenide would eliminate such a need.

And I assume that a lot of current and future users would benefit from it as well, since the current 4 second limit on the isDisplayed() method can sometimes be restrictive. In complex applications with dynamic content, the ability to specify a custom timeout is crucial for creating flexible and reliable tests.

@asolntsev
Copy link
Member

the current 4 second limit on the isDisplayed() method

Wait, this is wrong.
Method $.isDisplayed() does NOT have any timeout. It responses true/false immediately.

The method that has timeout 4 seconds is $.shouldBe(visible). And you can specify a custom timeout for it: $.shouldBe(visible, ofSeconds(8)).

asolntsev added a commit that referenced this issue Feb 4, 2024
This is the historical moment: finally, we allow writing IFs in tests ¯¯\_(ツ)_/¯¯
@asolntsev asolntsev linked a pull request Feb 4, 2024 that will close this issue
@asolntsev asolntsev self-assigned this Feb 4, 2024
@asolntsev asolntsev added this to the 7.1.0 milestone Feb 4, 2024
asolntsev added a commit that referenced this issue Feb 4, 2024
This is the historical moment: finally, we allow writing IFs in tests ¯¯\_(ツ)_/¯¯
asolntsev added a commit that referenced this issue Feb 6, 2024
@mbuchner
Copy link

+1 thanks for implementing / adding this feature!

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

Successfully merging a pull request may close this issue.

4 participants