have_element is a bit more confusing than be_on_page

[jwoertink]

In this PR https://github.com/luckyframework/lucky_flow/pull/135 be_on_page was removed in favor of have_element. This ended up being a breaking change. When trying to fix the specs, I found it really confusing on how to properly use this.

https://github.com/luckyframework/lucky_cli/blob/620ca03e9a783f3f70e2a5109765b8912f52cfb8/src/browser_authentication_app_skeleton/spec/support/flows/authentication_flow.cr#L29-L35

def should_be_signed_in
  sign_out_button.should be_on_page
end

def should_have_password_error
  el("body", text: "Password is wrong").should be_on_page
end

These end up becoming

def should_be_signed_in
  self.should have_element("@sign-out-button")
end

def should_have_password_error
  self.should have_element("body", text: "Password is wrong")
end

You could have the implicit self, but just having the floating should outside of a spec feels weird. The other thing is, before it read like the flow was expecting to find the element on the page. Now it reads as if the flow itself should have this element, but it's not really the "flow" object that has the element, it's the page that's being parsed by the flow.

I think for now, I'll create a shim for readability

private def current_page
  self
end

current_page.should have_element("@sigh-out-button")

Another nice update we could have is maybe an override that allows have_element to take a flow.el().

def should_be_signed_in
  current_page.should have_element(sign_out_button)
end

private def sign_out_button
 el("@sign-out-button")
end

[matthewmcgarvey]

The reason for the change in specs is that the should be_on_page thing was using lazy evaluation of elements. So you could writeel("body", text: "Password is wrong") and nothing would happen until you added the spec expectation to it. Now everything is eagerly loaded so you can't have expectations on an element class since the element would throw an error if it didn't exist.

[jwoertink]

Ah, nice. I didn't know that. Would we still be able to create an overload to pass in el() objects here?

I'm thinking we could maybe just rename the method to something like find_element or whatever.... Then it might look more like this:

def should_be_signed_in
  should find_element(sign_out_button)
end

private def sign_out_button
  el("@sign-out-button")
end

Just tossing out some ideas. This is low priority for now, so we can circle back to it. I just wanted to make sure I had it written down :smile:

[matthewmcgarvey]

I don't think so, currently. Anything's possible though so someone could add in lazy-loaded elements so that specs could go back to the way they were before. I could go hunting for the reason why it had to change, don't remember why off the top of my head though

[jwoertink]

I guess I'll need to spend a bit of time in this code base. I'm not really too familiar with how it all works. :sweat_smile: