Conversation
|
|
||
| kitty_page = kitty_class.new | ||
| capybara_stub.session.driver.expects(:execute_script) do |script| | ||
| puts script |
There was a problem hiding this comment.
Good catch, I was supposed to put assertions here about the contents of the script but missed it because the test passed! Now fixed.
|
|
||
| # Integration tests | ||
| include: | ||
| - env: RAILS_VERSION=4.0 |
There was a problem hiding this comment.
It is good that we are leaving 4.2 not only for ourselves but because this is open source.
There was a problem hiding this comment.
I'm not sure what you mean here. Are you saying you are glad I didn't get rid of 4.2, or are you suggesting we should keep 4.0?
|
|
||
| s.required_ruby_version = '>= 1.9.3' | ||
| s.required_ruby_version = '>= 2.2.5' | ||
| s.authors = ["Donnie Tognazzini"] |
There was a problem hiding this comment.
Good idea, changed to "AppFolio Engineering"
|
|
||
| def size | ||
| node.all(:xpath, item_xpath, options).size | ||
| node.all(:xpath, item_xpath, options.merge(wait: false)).size |
There was a problem hiding this comment.
I imagine this may cause some failures that will need to be updated to use the assert_eventually assertions.
There was a problem hiding this comment.
In Capybara 3.0 Element#all was changed to wait by default for at least one element.
The line here just maintains existing behavior by making it not wait. The Collection#size method needs to return the current size, whether it is 0 or above.
| node.first(*eval_locator(locator)) != nil | ||
| else | ||
| locator = eval_locator(loaded_locator) | ||
| if locator.empty? |
There was a problem hiding this comment.
I don't understand this change. We count it as loaded if there is no locator?
There was a problem hiding this comment.
Yes, that is maintaining existing behavior, I've just swapped the if/else so that rubocop doesn't complain about assigning a variable within an if statement.
| @locator == default_locator | ||
| end | ||
|
|
||
| def reload_descendents |
There was a problem hiding this comment.
The name reload_descendents and the comment seem to conflict with eachother. "descendent" indicates down to me, but we're going up? Maybe reload_predecessors, reload_parents, reload_ancestors?
There was a problem hiding this comment.
Good catch, I've renamed it to reload_ancestors
| end | ||
|
|
||
| implicit_element.__send__(name, *args, &block) | ||
| rescue Selenium::WebDriver::Error::StaleElementReferenceError |
There was a problem hiding this comment.
Normally we are forced to reload the appropriate component in the test. Am I correct in understanding you're removing the need for that?
There was a problem hiding this comment.
Yes, this has been a source of flakiness in tests since forever. Hopefully we should see a reduction in flakiness with this change.
pkmiec
left a comment
pkmiec
left a comment
There was a problem hiding this comment.
Thanks! I added a couple small comments as food for thought.
| ensure_loaded! | ||
| self | ||
| end | ||
|
|
There was a problem hiding this comment.
clever! what do you think about using aepos-reloading to reduce the chance of name collisions?
|
|
||
| addons: | ||
| firefox: "23.0" | ||
| firefox: "58.0.1" |
There was a problem hiding this comment.
just curious. have you tried FF 59.x?
There was a problem hiding this comment.
Looks like it works with FF 59.0.3.
https://travis-ci.org/appfolio/ae_page_objects/jobs/375049909
sarahsehr
left a comment
sarahsehr
left a comment
There was a problem hiding this comment.
You should definitely get another review, but other than what I indicated I didn't understand, this seems reasonable.
4 participants
This pull request adds support for Capybara 3.0.
In addition to this the following changes are made:
Document#reloadto allow a reliable method of reloading the current page without needing to create a new page object.StaleElementReferenceErrorcan occur on page reload