SitePrism should use Capybara's implicit wait functionality

[tmertens]

Natritmeyer,

I started using SitePrism fairly recently to rewrite existing Capybara tests in order to abstract page element locators from the individual test cases. In the relatively short time I have been working with it, I have found myself working around a number of test failures as a result of SitePrism explicitly overriding capybara's implicit wait functionality, and not passing through a sufficient amount of Capybara behavior to implement various Capybara search options in the tests.

Given that SitePrism is a wrapper for Capybara, I would expect that SitePrism would do its best to mimic and/or passthrough the default Capybara behavior wherever possible rather than replace it.

The biggest issue I have seen with Prism is its override of the Capybara implicit wait in methods such as has_#{elementname}?. This goes against the default Capybara behavior. See this article about why wait_until was removed from Capybara 2.0: http://www.elabs.se/blog/53-why-wait_until-was-removed-from-capybara Capybara's implicit waits are good and SitePrism shouldn't be overriding them without explicitly being told to by the tester.

Take for example this snippet of test code (this is a fabricated example to demonstrate a few different potential pitfalls of SitePrism):

feature 'My Index Page', js: true do
   scenario 'displays a list of articles' do
      my_index_page.load
      # See Issue 1 below
      campaigns_page.should have_articles
   end

   scenario 'displays multiple pages of articles' do
      my_index_page.load
     # This line does take advantage of capybara's implicit waits because current_page returns a capybara object:
      my_index_page.pagination.current_page.should have_content "Page 1 of 3"
      my_index_page.pagination.last_page_link.click

      # See Issue #2 below
      my_index_page.should be_displayed

     # See Issue #3 below
      my_index_page.articles.count.should eql 25
   end
end

Issue 1

If an element or section takes time to appear after page load, the method page.has_element_name? will fail because it explicitly sets capybara's wait for the block to 0, rather than allowing Capybara's implicit waiting behavior to pass through to SitePrism's behavior.

Issue 2

This issue is already addressed in PR #40 but is listed here for convenience.

page.displayed? fails because displayed? has no implicit wait, so if an action causes the page to redirect, then displayed? fails on the next expected page because the method hits a race condition between when the prior action occurred and when the next page is actually loaded in the browser.

Issue 3

We are not able to pass advanced Capybara selectors through site prism objects when attempting to access them.

The common example for this is, as in the above example, expecting some count of an elements or sections array to be returned. In the above case, we expect the count to equal 25; however, this presumes that when the my_index_page.articles array is returned that all 25 articles have already finished loading. With capybara we could tell it to expect 25 articles with the :count => 25 filter; however, with SitePrism this is not possible without predefining it in our page file; however, it is not always possible how many elements to expect in an array when the element object is created in the Page class, and it makes that element extremely inflexible when the number of elements returned are the dynamic result of something like a database query.

It would be ideal to be able to supply a base locator in the Page class when the element/section is defined, and when we later access that element/section to be able to supply additional capybara filters. For example:

page.should have_articles(:count => 25)

In this case the count filter should be appended to the default search parameters for the element and passed through to Capybara to ensure it implicitly waits for 25 articles to be displayed.

[natritmeyer]

Capybara's implicit waits are good and SitePrism shouldn't be overriding them without explicitly being told to by the tester.

Disagree: Capybara's implicit waits are sometimes good. I've found them to sometimes be a pain.

See this article about why wait_until was removed from Capybara 2.0

I'm well aware of why wait_until was taken out. But, I think that leaving me without it altogether was not sensible. I've discussed this with jnicklas (teamcapybara/capybara#806 (comment)) but it's his project and his decision.

Regarding issue 1:

When I wrote site_prism I wanted the ability to check if something existed on the page at the exact moment in time that my check ran. I didn't want to hang around waiting to see if it would appear, I wanted to know right now if the element existed on the page. Therefore has_element_name? explicitly sets the wait time to 0. I don't want to wait and then check, I want to check right now. If I want to wait before I check that something is on the page then I want to do that explicitly. Otherwise, has_element_name? is not has_element_name?; it would be better renamed wait_for_element_and_throw_exception_if_it_doesnt_appear_within_a_given_time. Which is exactly what calling element_name will do.

Capybara's implicit waits are cool, but they often get in my way. I very often want more granular control over what I wait for and what I don't than Capybara will give me.

Regarding issue 2:

When I get some time (and that won't be for a couple of weeks), I'll take a look.

Regarding issue 3:

Would be happy to take a look at a pull request for this! It would be cool to support this sort of thing.

Final thing: if site_prism doesn't work for you, don't use it! I will give you a full refund :) You state that you're using site_prism in order to abstract page element locators from the individual test cases - if that's all you want it for then you could get by with just having a bunch of constants in a bunch of modules.

@the_universe Why do I always end up in in-depth discussions about the design of site_prism at 1:30am when I'm on holiday?

[natritmeyer]

Capybara's implicit waits are good and SitePrism shouldn't be overriding them without explicitly being told to by the tester.

Disagree: Capybara's implicit waits are sometimes good. I've found them to sometimes be a pain.

See this article about why wait_until was removed from Capybara 2.0

I'm well aware of why wait_until was taken out. But, I think that leaving me without it altogether was not sensible. I've discussed this with jnicklas (teamcapybara/capybara#806 (comment)) but it's his project and his decision.

Regarding issue 1:

When I wrote site_prism I wanted the ability to check if something existed on the page at the exact moment in time that my check ran. I didn't want to hang around waiting to see if it would appear, I wanted to know right now if the element existed on the page. Therefore has_element_name? explicitly sets the wait time to 0. I don't want to wait and then check, I want to check right now. If I want to wait before I check that something is on the page then I want to do that explicitly. Otherwise, has_element_name? is not has_element_name?; it would be better renamed wait_for_element_and_throw_exception_if_it_doesnt_appear_within_a_given_time. Which is exactly what calling element_name will do.

Capybara's implicit waits are cool, but they often get in my way. I very often want more granular control over what I wait for and what I don't than Capybara will give me.

Regarding issue 2:

When I get some time (and that won't be for a couple of weeks), I'll take a look.

Regarding issue 3:

Would be happy to take a look at a pull request for this! It would be cool to support this sort of thing.

Final thing: if site_prism doesn't work for you, don't use it! I will give you a full refund :) You state that you're using site_prism in order to abstract page element locators from the individual test cases - if that's all you want it for then you could get by with just having a bunch of constants in a bunch of modules.

@the_universe Why do I always end up in in-depth discussions about the design of site_prism at 1:30am when I'm on holiday?

[natritmeyer]

What do you think to SitePrism providing a config block that will allow this sort of thing to be configured? Eg:

SitePrism.config do |c|
  c.existence_check_should_wait = true #default is false
end

...which could be picked up in element_container.rb like so:

def create_existence_checker(element_name, *find_args)
  method_name = "has_#{element_name.to_s}?"
  create_helper_method method_name, *find_args do
    define_method method_name do
      wait_time = SitePrism.existence_check_should_wait ? Capybara.default_wait_time : 0
      Capybara.using_wait_time wait_time do
        element_exists? *find_args
      end
    end
  end
end

That would preserve the current SitePrism functionality that I want, and would also allow Capybara's functionality to be exposed for people who want it...

Thoughts?

[natritmeyer]

What do you think to SitePrism providing a config block that will allow this sort of thing to be configured? Eg:

SitePrism.config do |c|
  c.existence_check_should_wait = true #default is false
end

...which could be picked up in element_container.rb like so:

def create_existence_checker(element_name, *find_args)
  method_name = "has_#{element_name.to_s}?"
  create_helper_method method_name, *find_args do
    define_method method_name do
      wait_time = SitePrism.existence_check_should_wait ? Capybara.default_wait_time : 0
      Capybara.using_wait_time wait_time do
        element_exists? *find_args
      end
    end
  end
end

That would preserve the current SitePrism functionality that I want, and would also allow Capybara's functionality to be exposed for people who want it...

Thoughts?

[ronwsmith]

👍 to the config option.

[ronwsmith]

👍 to the config option.

[tmertens]

+1 My thought exactly!

[tmertens]

+1 My thought exactly!

[tmertens]

This is resolved.

[tmertens]

This is resolved.