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

Make elements returned by all and first reloadable #1439

Closed

Conversation

abotalov
Copy link
Collaborator

@abotalov abotalov commented Nov 7, 2014

Should fix #1410

All elements are considered reloadable. #allow_reload! may be removed from Element and Simple in next major version as it's not needed with this PR.

One of moments to discuss - currently behavior may be unexpected e.g. in the following case:

el = all('li')[-1] # there are 4 'li' elements on the page
find('.remove_last_li').click
expect { el.text } not_to raise_error # I believe that will raise an error as that will search for third element, not for the last one

But I don't know at the moment a good solution for it. The problem is that element of enumerable doesn't know the way using which it was got from that enumerable.

@twalpole
Copy link
Member

twalpole commented Dec 8, 2014

Sorry its taken so long for me to look at this -- work and life have been in the way. The potential unexpected behavior around multiple results is concerning to me, and is most likely why all results weren't made reloadable in the first place - I think this needs a lot more thought before inclusion.

@jnicklas
Copy link
Collaborator

This was precisely why I didn't make first and all reloadable in the first place, yes. You cannot be sure that the list has the same number of elements in the same order anymore, so you might end up interacting with something in a completely different area of the page. This is very unsafe and probably not at all what the user expects.

Suppose you have a list of messages where messages appear at the top. Now you do this:

messages = all(".messages")
hello = messages.find { |m| m.text.include?("Hello") }
hello.click_on("Reply")
fill_in "Message", with: "World"
click_button "Send"
expect(hello).to have_selector("1 reply")

Will this test succeed? It depends on the implementation. That's pretty bad. Essentially the behaviour of this test is completely undefined. I think it should be fairly obvious why: hello moves as the new message is added. Reloading it, even if we preserve the index of it, makes absolutely no sense. We'll end up with a handle to a different message.

Huge 👎 from me. I spent a lot of time thinking about this stuff when I designed the reloading/waiting system, and I still am very sure that reloading these elements is a very flawed idea, and will lead people to writing less stable tests like the one above. The above test could have easily been written in idiomatic Capybara style and it would have worked just fine:

hello = find(".messages li", text: "Hello")
hello.click_on("Reply")
fill_in "Message", with: "World"
click_button "Send"
expect(hello).to have_selector("1 reply")

@twalpole
Copy link
Member

Closing due to @jnicklas huge 👎 and my misgivings about the potential unexpected behavior

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

Successfully merging this pull request may close these issues.

Make elements returned by all and first reloadable
3 participants