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

ElementsCollection object does not 'refresh' itself when calling get() or size() #181

Closed
yashaka opened this issue May 14, 2015 · 7 comments
Labels

Comments

@yashaka
Copy link

yashaka commented May 14, 2015

If you store SelenideElement object in the variable like here:

ElementsCollection tasks = $$("#todo-list>li")

Then calling should will refresh elements collection object (i.e. search again for all elements matching locator on the page).

But calling get() or size will not refresh the object.

This make impossible an implementation of some "BDD style steps" when you combine action and assertion of its results in one step-method

Example:

public class BaseTest {

    public static ElementsCollection tasks = $$("#todo-list>li");

    @Test
    public void testPasses(){
        open("http://todomvc.com/examples/troopjs_require/#");

        $("#new-todo").setValue("a").pressEnter();
        tasks.shouldHave(size(1));

        $("#new-todo").setValue("b").pressEnter();
        tasks.shouldHave(size(2));

        tasks.shouldHave(texts("a", "b"));
    }

    @Test
    public void testFailes(){
        open("http://todomvc.com/examples/troopjs_require/#");

        $("#new-todo").setValue("a").pressEnter();
        assertEquals(1, tasks.size());

        $("#new-todo").setValue("b").pressEnter();
        assertEquals(2, tasks.size()); /* 
            => java.lang.AssertionError: Expected :2 Actual :1 
        */

        tasks.shouldHave(texts("a", "b"));
    }

    @Test
    public void testStillPasses(){
        open("http://todomvc.com/examples/troopjs_require/#");

        $("#new-todo").setValue("a").pressEnter();
        assertEquals(1, $$("#todo-list>li").size());

        $("#new-todo").setValue("b").pressEnter();
        assertEquals(2, $$("#todo-list>li").size());

        tasks.shouldHave(texts("a", "b"));
    }

    @Test
    public void testReasonWhyThisIsNeededForBDDStyle() {
        //given
        open("http://todomvc.com/examples/troopjs_require/#");

        //then
        addTask("a");

        //then
        addTask("b");
    }

    // This is kind of BDD step with action + assert in one helper
    public void addTask(String taskText){
        $("#new-todo").setValue("1").pressEnter();
        tasks.get(tasks.size() - 1).shouldHave(exactText(taskText));
        /*
            by the way, why there is no "last()" method in ElementsCollection ?
         */
    }
}

Full code: https://www.refheap.com/101119

@asolntsev
Copy link
Member

Because loading of the entire collection can be slow, the ElementsCollection class was initially designed so that it reuses previously loaded element. That's why method tasks.size() returns the same result at every call. Otherwise tests can be very slow in case of big collections. If you need the "re-try" behaviour, use method $$.shouldHave(size(2)).

So, I am not sure it's a bug. Fixing this issue may cause performance downgrade.

@yashaka
Copy link
Author

yashaka commented Jun 15, 2015

What about $$.get ?
Does it work the same like size?
If yes then at least it's a consistent behaviour...

But how then the following code work:

        open("http://google.com/ncr");
        $(By.name("q")).setValue("selenium").pressEnter();
        ElementsCollection results = $$(".st");
        assert results.size() == 0;
        results.get(9).shouldHave(text("Downloading Selenium server"));

If $$.get reloads the ElementsCollection, why then not to make size to reload for consistency?

And why not to make this "optional" via Configuration for those who need this? :)

@yashaka
Copy link
Author

yashaka commented Jun 15, 2015

Hm... Seems like get is not like size...

$.get at least can know what to wait... - the element index for which is passed as param...

Hm... Nevertheless... currently the only way to fix "bdd step with check" problem is to use tasks as method not variable... What is not handy and not consistent...

I think that then at least the $.refresh method should be added for manual refresh in such cases...
Though I think that make refresh configurable via Configuration (even turned off by default) - is also a good idea...

About performance... So far I can hardly imagine the drawback taking into account that "calling raw $.size" will be so often in tests... Maybe it's a good idea to do some "assessment"...

@asolntsev
Copy link
Member

I think the "bdd step with check" problem is not a real problem.
I believe it's quite natural to create a new variable for new collection that have been changed.
It's not connected to BDD in any way.

In Selenide, it's natural to use methods tasks.shouldHave(size(1)), tasks.shouldHave(size(2)) like described in your testPasses() example. Let's use this pattern.

@yashaka
Copy link
Author

yashaka commented Jun 18, 2015

Sorry that I have not given up yet.

I called it "BDD" check, just becuase sometimes in BDD you want to write a "step" that does two things: action/operation + check of its result. In addition this "check" should be "general", it should work for different contexts.
I found the only way to check result of "adding item to the list" - get the last item in the "list with added item" and check it.

Can you help to build such "step with check" (let's even forget about "bdd" word) with selenide? :)

The pattern you recommend will give something like this:

1

    public void addTask(String taskText){
        int oldSize = tasks.size
        $("#new-todo").setValue("1").pressEnter();
        tasks.get(oldSize).shouldHave(exactText(taskText));

    }

or

2

    public void addTask(String taskText){
        int oldSize = tasks.size
        $("#new-todo").setValue("1").pressEnter();
        int newSize = oldSize + 1
        tasks.shouldHave(size(newSize));
        tasks.get(newSize - 1).shouldHave(exactText(taskText));
    }

or are there better options?

And now look into these examples - 1 and 2 - once more.
IMHO the most part of users, when see such code, will gain a thought that "tasks" - are SMART and refresh themselves...
How otherwirse after
tasks.shouldHave(size(newSize));
here
tasks.get(newSize - 1)
we will have new updasted content inside tasks?
and when now we call

    tasks.size()

We will get newSize not oldSize.

Yeah, under cover, the tasks are not smart... But they look like smart - and this confuses.

I want to say that current Selenide behavior is not consistent and confuses the user.

Now about the "philosophy".
I think in the case of UI automation - it's much more obvious for the user to think about "tasks" - as not variable with collection that if changed need another variable - but "a real tasks in the browswer page" that can be updated of course.
Such approach is more "user friendly", and IMHO more "OOP" style, when we try to emulate with a code - the real world. In real world - "tasks as entities are smart"

P.S. 1
/*
by the way, why there is no last() method in ElementsCollection ?
*/

P.S. 2
I like the Selenium's PageFactory approach. Where you will create your "tasks" with @FindBy as "smart and automatically refreshed" all the time by default, and when you want - you can "cache" the tasks, so they will be not refreshed and you can win in performance if you need this.

Seems like Selenide can't give the alternative to Selenium in this context.

P.S. 3

About performance...

You said that we will loose in performance... But think - when will we use the size ?
Very rarely, and in such contexts like tasks.get(tasks.size - 1), where we most probably in truth
need:

    tasks.shouldHave(size(newSize));
    tasks.get(newSize - 1).shouldHave(exactText(taskText));

So we definitly need to wait... And this is not a "waste of time", because we "want to wait here". We just have to do this explicitly, not like in other "selenide" contexts, when we have "implicit smart waits" by default.

When I write tests with Selenide - I really rarely use size. And when I use (like in example above) I want to wait. So... I do not see any lost in performance when we make size the same way smart as get and others. Once more - this "difference", even taking into account that get is not like size, - confuses the user (hope not only me... (: )...
When you used to see that get updates the object's content, then when you see that size does not - you are confused:)

@yashaka yashaka changed the title SelenideElement object does not 'refresh' itself when calling get() or size() ElementsCollection object does not 'refresh' itself when calling get() or size() Sep 16, 2015
@yashaka
Copy link
Author

yashaka commented Aug 6, 2016

The issue described here happened to be valuable for one more person here: http://automated-testing.info/t/pomogite-razobratsya-s-problemoj-czikla-v-selenide/10467

Here is a bit more "research" describing the problem of "ElementsCollection's size(), get(int), and iterator" being not working as expected by users (i.e. being up to date at any time we ask the ElementsCollection object about something):

import com.codeborne.selenide.ElementsCollection;
import com.codeborne.selenide.SelenideElement;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

import static com.codeborne.selenide.CollectionCondition.empty;
import static com.codeborne.selenide.Selenide.*;

public class IteratingElementsCollectionShouldBeUpToDateTest {

    @Before
    public void openPage(){
        open("http://todomvc4tasj.herokuapp.com/");
    }

    @After
    public void clearData(){
        executeJavaScript("localStorage.clear()");
    }

    @Test
    public void WhenUsingIterator_OfSameElementsCollectionObject(){

        $("#new-todo").setValue("a").pressEnter();
        $("#new-todo").setValue("b").pressEnter();
        $("#new-todo").setValue("c").pressEnter();

        for (SelenideElement task : $$("#todo-list>li")) {
            task.hover().find(".destroy").click();
        }

        $$("#todo-list>li").shouldBe(empty);
        /*
         * removes only a & c, leaving b, and then FAILS here with:
            ListSizeMismatch : expected: = 0, actual: 1, collection: #todo-list>li
            Elements: [
                <li class="active" data-index="1" value="0">b</li>
            ]

            Screenshot: file:/Users/ayia/projects/tmp/selenide-tests/build/reports/tests/1470479873538.0.png
            Timeout: 6 s.
         */
    }

    @Test
    public void WhenUsingGet_OnNewElementsCollectionObject(){
        $("#new-todo").setValue("a").pressEnter();
        $("#new-todo").setValue("b").pressEnter();
        $("#new-todo").setValue("c").pressEnter();

        for (SelenideElement task : $$("#todo-list>li")) {
            task.hover().find(".destroy").click();
        }

        while (!$$("#todo-list>li").isEmpty()) {
            $$("#todo-list>li").get(0).hover().find(".destroy").click();
        }

        $$("#todo-list>li").shouldBe(empty);

        /*
         * PASSED
         */
    }

    @Test
    public void WhenUsingGet_OnSameElementsCollectionObject_WithIterationBasedOnIsEmpty(){
        $("#new-todo").setValue("a").pressEnter();
        $("#new-todo").setValue("b").pressEnter();
        $("#new-todo").setValue("c").pressEnter();

        for (SelenideElement task : $$("#todo-list>li")) {
            task.hover().find(".destroy").click();
        }

        ElementsCollection tasks = $$("#todo-list>li");

        while (!tasks.isEmpty()) {
            tasks.get(0).hover().find(".destroy").click();
            /*
             * removes all and then FAILS here with 
                   java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
             */
        }

        tasks.shouldBe(empty);
    }
}

Current workaround for this issues is described in a WhenUsingGet_OnNewElementsCollectionObject test above.
The idea is to use new ElementsCollection object for any "asking collection about something". This can be achieved either via using straightforward $$ syntax or by abstracting ElementsCollection into method instead of variable, i.e using

ElementsCollection tasks() { 
    return $$("#todo-list li"); 
}

instead of

ElementsCollection tasks = $$("#todo-list li");

@BorisOsipov
Copy link
Member

Close old issue. Feel free to reopen and start disscussion again if it still needs to implement.

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

No branches or pull requests

3 participants