Conversation
tlconnor
left a comment
tlconnor
left a comment
There was a problem hiding this comment.
I found a few small things, but otherwise this looks good to go.
| @@ -1,53 +0,0 @@ | |||
| module AePageObjects | |||
There was a problem hiding this comment.
Are these file removals meant to be part of this commit or the next?
There was a problem hiding this comment.
Yes, that got messed up a bit. I squashed most of it together now with a better msg.
| def wait_until(seconds_to_wait = nil, error_message = nil) | ||
| seconds_to_wait ||= default_max_wait_time | ||
| start_time = Time.now | ||
| def wait_until(seconds_to_wait = nil, error_message = nil, &block) |
There was a problem hiding this comment.
You've called this commit pk - passing tests but really this one contains the bulk of the changes.
There was a problem hiding this comment.
Reworded the commit msg.
| def wait_until(seconds_to_wait = nil, error_message = nil, &block) | ||
| @wait_until ||= 0 | ||
| @wait_until += 1 | ||
| # start_time = Time.now |
There was a problem hiding this comment.
Are these commented lines needed anymore?
|
|
||
| result | ||
| ensure | ||
| # puts "#{"--" * @wait_until} wait_until => #{Time.now - start_time}" |
| @@ -1,3 +1,3 @@ | |||
| module AePageObjects | |||
| VERSION = '2.1.0'.freeze | |||
| VERSION = '2.0.1'.freeze | |||
There was a problem hiding this comment.
Why was the version number decreased? Given the API changes we'll need a major version number, making this 3.0.0
There was a problem hiding this comment.
You're right. This should not have changed here. Once it is merged, we can bump to 3.0, update changelog, and release.
| @@ -1,5 +1,6 @@ | |||
| ENV['RAILS_ENV'] = 'test' | |||
|
|
|||
| puts $LOAD_PATH | |||
pkmiec
force-pushed
the
pkImproveWaiting
branch
from
305a30d to
a9323b0
Compare
pkmiec
force-pushed
the
pkImproveWaiting
branch
from
a9323b0 to
d5fe33c
Compare
2 participants
There are 2 main problems with AePageObjects making it prone to flakiness,
Capybara.using_wait_time(0)preventing Capybara from retrying errors and reloading nodes. The page polling also does not catch any errors itself causing exceptions to be potentially raised prior to waiting the asked for time. The page polling is usedbrowser.find_document,window.change_to, andabsent?/present?/ and friends on element proxy.ensure_loaded!is difficult to write / reason about as it is sometimes called inside of page polling block (see above) and sometimes it is not. This means that implementors have to know about all the exceptions that should be caught and usingAePageObjects#wait_untilis not good because it can blocks for longer than what is intended by page polling.This PR improves AePageObjects by,
#wait_until. It now rescues and retries all relevant selenium / capybara / ae_page_objects exceptions. It only throws 'WaitTimeoutError' (which includes the original cause) making it easy for user to rescue errors. It lowers Capybara's default time allowing capybara to retry / reload, but also allowing errors to be handled at the higher level (and thus another chance to success). It allows itself to be called multiple times which the top-level invocation being in charge of the timeout.ensure_loaded!now callsis_loaded?which is meant to quickly return an answer. Subclasses should overrideis_loaded?now, but no waiting / rescuing is needed.#all/#findfrom being delegated to Capybara node. These return Capybara nodes (and in case of all not reloadable once) and it is better to be explicit about their use. Also avoids confusion with#findprovided byAePageObjects::Colleciton.