Respect options in item_locator when looking up items in collections by tlconnor · Pull Request #189 · appfolio/ae_page_objects

tlconnor · 2016-09-16T19:30:25Z

Currently any options passed in item_locator for collections are ignored.

For example, given a collection where we only want the visible elements:

collection :children, item_locator: ['li', { visible: true }]

where the markup looks something like this:

<ul>
  <li>First Child</li>
  <li style="display:none">Second Child</li>
  <li>Third Child</li>
</ul>

then children.size will run the query [:xpath, './/li'] to look up children and [:xpath, './/li[2]'] to find the second child. This causes two problems:

  children.size == 3 # should be 2
  children[1].text == 'Second Child' # shoud be 'Third Child'

This PR changes the behavior to pass options through when looking up children. That means now children.size will run the query [:xpath, './/li', { visible: true }] to look up children and [:xpath, './/li[2]', { visible: true }] to find the second child. So now

  1. children.size == 2
  2. children[1].text == 'Third Child'

bboe

We discussed and agreed on a few changes in person. Otherwise, this looks great.

bboe · 2016-09-19T17:48:50Z

-
-        query_args = evaled_locator + [{:exact => true}]
-        query = Capybara::Query.new(*query_args)
+        evaled_locator = eval_locator(@item_locator).clone


What purpose does clone solve here?

See previous comment / updated code

bboe · 2016-09-19T17:49:22Z

      @item_xpath ||= begin
-        evaled_locator = eval_locator(@item_locator)
-
-        query_args = evaled_locator + [{:exact => true}]


{:exact => true} is this no longer needed?

I have created a separate PR for the {:exact => true} change. It turned out it was needed but was not obvious from the code. See #190 for details.

bboe · 2016-09-19T17:53:45Z


    def size
-      node.all(:xpath, item_xpath).size
+      node.all(:xpath, item_xpath, options).size


While it looks like this may have always been an issue, but is this result cached at all? Given that size is called for each at call it seems like there may be some room for optimization.

I've created a separate PR for the performance improvement #191

bboe · 2016-09-19T17:56:11Z


+    def options
+      @options ||= begin
+        evaled_locator = eval_locator(@item_locator).clone


Why is clone being used here? and on the next line?

The intention here was to prevent the original object from being modified when adding the { exact: true } option. Please have a look at the new version of the code which now uses dup instead.

bboe · 2016-09-19T17:57:22Z

-        query_args = evaled_locator + [{:exact => true}]
-        query = Capybara::Query.new(*query_args)
+        evaled_locator = eval_locator(@item_locator).clone
+        evaled_locator.pop if evaled_locator.last.is_a?(::Hash)


What is the content that is being removed if it is a hash? How does it differ from options? If they're the same, why bother doing it in this way?

Please see the latest code. This makes more sense now that {exact: true} is being added

rmacklin · 2016-09-19T18:06:19Z


+    def options
+      @options ||= begin
+        evaled_locator = eval_locator(@item_locator).clone


For consistency, I think this, and other instances, should use dup rather than clone. (There is also some extra overhead with clone because it does more than dup, and in this case the extra things it does aren't necessary.)

rmacklin · 2016-09-19T18:17:25Z

-        query = Capybara::Query.new(*query_args)
+        evaled_locator = eval_locator(@item_locator).clone
+        evaled_locator.pop if evaled_locator.last.is_a?(::Hash)
+        query = Capybara::Query.new(*evaled_locator, options)


It looks like this change loses the :exact => true default. That's a breaking change. I also think it's an undesirable change because it's a good default for collection items.

Fixed in #190

rmacklin · 2016-09-19T18:26:31Z

-        query = Capybara::Query.new(*query_args)
+        evaled_locator = eval_locator(@item_locator).clone
+        evaled_locator.pop if evaled_locator.last.is_a?(::Hash)
+        query = Capybara::Query.new(*evaled_locator, options)


Also, this logic seems confusing. We're dropping the last element of evaled_locator if it's a hash, and then just adding it back on via options? It seems more straightforward to add an options hash if the last element wasn't already one. That's what the old code was doing, but it wasn't merging the hash if one was already there. Can we just do that instead? What if we override eval_locator in this class? Would that make this implementation simpler?

See previous comment in reply to @bboe

tlconnor · 2016-09-19T21:10:06Z

@rmacklin @bboe thanks for your feedback on this. I've made the required changes and this is ready for re-review.

bboe · 2016-09-19T21:13:58Z

-        query_args += [{:exact => true}] if query_args.first.to_sym == :xpath
+        if query_args.first.to_sym == :xpath
+          query_args.pop if query_args.last.is_a?(::Hash)
+          query_args.push options


Is there any case where options might want to have exact: false? If so this overwrites the options.

Perhaps something like:

if query_args.last.is_a?(::Hash) query_args.last.update(options) else query_args.push exact: true end

I find this a little easier to read as well.

And my logic was way off. I like what you suggested on slack.

Good point, if the user specifies a value for exact it should be respected. Now fixed.

We should add a test for the case where exact was specified by the user

Added https://github.com/appfolio/ae_page_objects/pull/189/files#diff-7b6f0d059c004fe55b277732708baf53R101

rmacklin · 2016-09-19T21:36:11Z

    end

+    def options
+      evaled_locator = eval_locator(@item_locator)


In the previous changeset, you were memoizing the result of this method (6895134#diff-4edadc6ec811dffb9a89b8cc6d88feccR60). Was that intentionally removed?

Yeah, I intentionally made it return a new object every time so that the result was safe to mutate. There is no real performance gain to be had by memoizing here.

rmacklin · 2016-09-19T21:47:09Z

+          # See https://github.com/jnicklas/capybara#exactness for more information.
+          #
+          query_args.push({}) unless query_args.last.is_a?(::Hash)
+          query_args.last[:exact] = true unless query_args.last.key?(:exact)


Previously you had been careful to avoid modifying the caller's parameters, but I think this line no longer does so (meaning that references they had to the query_args.last object would now have exact: true, which is undesirable).

I think you could do something like:

if query_args.last.is_a?(::Hash) query_args[-1] = query_args.last.dup else query_args.push({}) end query_args.last[:exact] = true unless query_args.last.key?(:exact)

Or...

default_options = {:exact => true} if query_args.last.is_a?(::Hash) query_args[-1] = default_options.merge(query_args.last) else query_args.push(default_options) end

Good point, I had done this query_args = eval_locator(@item_locator).dup but that only duplicates the Array, keeping the same entries within the array. I've updated this using your second suggestion.

I snuck in an edit to my second suggestion which extracts a default_options variable for reuse and readability, but it looks like you had already made the change. Mind using the local variable?

Also, I intentionally used the old hash syntax in that example for consistency with the rest of ae_page_objects. I think it would be better if we didn't introduce the new syntax in this PR.

And I still see new hash syntax being introduced in the diff

Updated with the default_options suggestion.

I don't agree with the hash syntax statement, the repo already has instances of the ruby 2.0 hash syntax and there is no problem with having a mix of both types while code is being migrated from the old syntax to the new.

There is a good reason for keeping the old syntax: ae_page_objects ~> 1.x supports Ruby 1.8.7, and sometimes changes made to master need to be backported to 1.x. Any changes with the new hash syntax cannot be directly cherry-picked to 1.x.

Can you show me the existing instances of the new syntax you've found?

If we need to support ruby 1.8.7 that makes sense.

Here is an example I found of the new hash syntax:
https://github.com/appfolio/ae_page_objects/blob/master/lib/ae_page_objects/elements/collection.rb#L60

(Irrelevant to this PR) I don't think we should be afraid to use 1.9+ only stuff here. This version no longer supports 1.8, and so if something needs to be backported, it can be fixed when porting. Otherwise functionality requiring 1.9+ could never be introduced if that is the logic.

Futhermore, ruby 1.8 has been EOL since July 21, 2014 (https://www.ruby-lang.org/en/news/2014/07/01/eol-for-1-8-7-and-1-9-2/) and 1.9 EOL since February 23, 2015 (https://www.ruby-lang.org/en/news/2014/01/10/ruby-1-9-3-will-end-on-2015/). As a result, neither of these versions are something we should continue to put any effort into supporting unless we had the need to for our own legacy code -- I don't believe any such code exists.

bboe · 2016-09-19T22:05:53Z

+          # See https://github.com/jnicklas/capybara#exactness for more information.
+          #
+          if query_args.last.is_a?(::Hash)
+            query_args[query_args.size - 1] = query_args.last.dup


query_args[-1] should work here.

rmacklin · 2016-09-19T22:16:02Z

+      assert_equal bullet1_stub, magazine.at(0).node
+    end
+
+    def test_locator_options__exact_false


Can you move this test to be after test_xpath_item_locator__matches_exact_text? And perhaps the test names should be something like test_xpath_item_locator__matches_exact_text_by_default and test_xpath_item_locator__exactness_can_be_overridden.

I appreciate the feedback you've given so far, but this is now going beyond what a reasonable code review should be. The names of the tests are good enough and the ordering looks logical enough to me.

I would like to see this merged if there are no other important issues to be fixed.

I don't think this is unreasonable for code review. What I've suggested makes things clearer. This test is an extension of the test coverage for the test_xpath_item_locator__matches_exact_text that was added in #190, so it makes sense to be placed after that one and named accordingly.

Its also related to the test_locator_options__ tests as it tests the impact of passing options. This test in question could be grouped with either set of tests.

The assertion the test makes is what makes it more like test_xpath_item_locator__matches_exact_text which has a parallel assertion. It doesn't follow the assertion format for the test_locator_options__ tests.

bboe suggested changes Sep 19, 2016

View reviewed changes

rmacklin reviewed Sep 19, 2016

View reviewed changes

tlconnor force-pushed the cpAddOptionsForCollection branch from 6895134 to 7847e9b Compare September 19, 2016 20:56

bboe suggested changes Sep 19, 2016

View reviewed changes

tlconnor force-pushed the cpAddOptionsForCollection branch from 7847e9b to 3eb09db Compare September 19, 2016 21:32

rmacklin reviewed Sep 19, 2016

View reviewed changes

tlconnor force-pushed the cpAddOptionsForCollection branch from 3eb09db to aea8056 Compare September 19, 2016 21:37

rmacklin suggested changes Sep 19, 2016

View reviewed changes

tlconnor force-pushed the cpAddOptionsForCollection branch 2 times, most recently from a7ef04c to 1cde0f0 Compare September 19, 2016 22:03

bboe reviewed Sep 19, 2016

View reviewed changes

rmacklin reviewed Sep 19, 2016

View reviewed changes

tlconnor force-pushed the cpAddOptionsForCollection branch from 1cde0f0 to 3a502a8 Compare September 19, 2016 22:17

bboe approved these changes Sep 19, 2016

View reviewed changes

respect options in item_locator when looking up items in collections

a8c8634

tlconnor force-pushed the cpAddOptionsForCollection branch from 3a502a8 to a8c8634 Compare September 19, 2016 23:15

rmacklin approved these changes Sep 19, 2016

View reviewed changes

rmacklin merged commit d100686 into master Sep 19, 2016

rmacklin deleted the cpAddOptionsForCollection branch September 19, 2016 23:32

Conversation

tlconnor commented Sep 16, 2016 • edited by rmacklin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bboe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmacklin Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmacklin Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmacklin Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlconnor commented Sep 19, 2016

Uh oh!

bboe Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmacklin Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmacklin Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmacklin Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tlconnor commented Sep 16, 2016 •

edited by rmacklin

Loading

rmacklin Sep 19, 2016 •

edited

Loading

rmacklin Sep 19, 2016 •

edited

Loading

rmacklin Sep 19, 2016 •

edited

Loading

bboe Sep 19, 2016 •

edited

Loading

rmacklin Sep 19, 2016 •

edited

Loading

rmacklin Sep 19, 2016 •

edited

Loading

rmacklin Sep 19, 2016 •

edited

Loading

rmacklin Sep 19, 2016 •

edited

Loading