From be216406e24a1d091ce1927b865f27cb061d046e Mon Sep 17 00:00:00 2001 From: Chris Horuk Date: Fri, 21 Jul 2017 17:24:09 -0700 Subject: [PATCH 1/2] ch - fix bug in item xpath generation Without the parentheses, this xpath selector calculation is incorrect. As an example, lets say the item_xpath evaluates as ".//someLocator[1]". What this selector is actually specifying is: find all someLocator elements under the current node that are the first child of their parent element. What we really want to say is: find exactly the first someLocator element. In order to get this behavior, we need to wrap the part of the locator that is not the array indexing in parentheses, due to the order of precendence of the xpath operators ([] has higher precedence than //). --- lib/ae_page_objects/elements/collection.rb | 2 +- test/unit/collection_test.rb | 16 +++++++-------- test/unit/dsl/collection_test.rb | 24 +++++++++++----------- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/lib/ae_page_objects/elements/collection.rb b/lib/ae_page_objects/elements/collection.rb index 0975e2b3..feb0f435 100644 --- a/lib/ae_page_objects/elements/collection.rb +++ b/lib/ae_page_objects/elements/collection.rb @@ -106,7 +106,7 @@ def item_xpath end def item_locator_at(index) - [:xpath, "#{item_xpath}[#{index + 1}]", options] + [:xpath, "(#{item_xpath})[#{index + 1}]", options] end def default_item_locator diff --git a/test/unit/collection_test.rb b/test/unit/collection_test.rb index 9a1e5f28..381fdb20 100644 --- a/test/unit/collection_test.rb +++ b/test/unit/collection_test.rb @@ -97,7 +97,7 @@ def test_locator_options__blank bullet1_stub = mock(:allow_reload!) magazine_node.expects(:all).with(:xpath, 'item_xpath', {}).returns([bullet1_stub]) - magazine_node.expects(:first).with(:xpath, 'item_xpath[1]', {}).returns(bullet1_stub) + magazine_node.expects(:first).with(:xpath, '(item_xpath)[1]', {}).returns(bullet1_stub) assert_equal bullet1_stub, magazine.at(0).node end @@ -116,7 +116,7 @@ def test_locator_options__present bullet1_stub = mock(:allow_reload!) magazine_node.expects(:all).with(:xpath, 'item_xpath', { :capybara => 'options' }).returns([bullet1_stub]) - magazine_node.expects(:first).with(:xpath, "item_xpath[1]", { :capybara => 'options' }).returns(bullet1_stub) + magazine_node.expects(:first).with(:xpath, "(item_xpath)[1]", { :capybara => 'options' }).returns(bullet1_stub) assert_equal bullet1_stub, magazine.at(0).node end @@ -173,8 +173,8 @@ def test_non_empty assert_equal 2, magazine.size - magazine_node.expects(:first).with(:xpath, "item_xpath[1]", {}).returns(bullet1_stub) - magazine_node.expects(:first).with(:xpath, "item_xpath[2]", {}).returns(bullet2_stub) + magazine_node.expects(:first).with(:xpath, "(item_xpath)[1]", {}).returns(bullet1_stub) + magazine_node.expects(:first).with(:xpath, "(item_xpath)[2]", {}).returns(bullet2_stub) each_block_call_count = 0 magazine.each do |bullet| bullet.name @@ -182,16 +182,16 @@ def test_non_empty end assert_equal 2, each_block_call_count - magazine_node.expects(:first).with(:xpath, "item_xpath[1]", {}).times(2).returns(bullet1_stub) + magazine_node.expects(:first).with(:xpath, "(item_xpath)[1]", {}).times(2).returns(bullet1_stub) assert_equal bullet1_stub, magazine.at(0).node assert_equal bullet1_stub, magazine.first.node - magazine_node.expects(:first).with(:xpath, "item_xpath[2]", {}).times(2).returns(bullet2_stub) + magazine_node.expects(:first).with(:xpath, "(item_xpath)[2]", {}).times(2).returns(bullet2_stub) assert_equal bullet2_stub, magazine.at(1).node assert_equal bullet2_stub, magazine.last.node - magazine_node.expects(:first).with(:xpath, "item_xpath[1]", {}).returns(bullet1_stub) - magazine_node.expects(:first).with(:xpath, "item_xpath[2]", {}).returns(bullet2_stub) + magazine_node.expects(:first).with(:xpath, "(item_xpath)[1]", {}).returns(bullet1_stub) + magazine_node.expects(:first).with(:xpath, "(item_xpath)[2]", {}).returns(bullet2_stub) assert_equal [bullet1_stub, bullet2_stub], magazine.map(&:node) assert_equal nil, magazine.at(1000) diff --git a/test/unit/dsl/collection_test.rb b/test/unit/dsl/collection_test.rb index 627e4753..1a0f6b6a 100644 --- a/test/unit/dsl/collection_test.rb +++ b/test/unit/dsl/collection_test.rb @@ -25,7 +25,7 @@ def test_collection__no_is__no_contains__block first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element_with_intermediary_class(previous_owners, 0, AePageObjects::Element, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) @@ -62,7 +62,7 @@ def test_collection__is__no_contains__block first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element_with_intermediary_class(previous_owners, 0, previous_owners_class.item_class, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) @@ -98,7 +98,7 @@ def test_collection__is__no_contains__block__no_item_class first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element_with_intermediary_class(previous_owners, 0, previous_owners_class.item_class, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) @@ -136,7 +136,7 @@ def test_collection__is__no_contains__no_block first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element(previous_owners, 0, previous_owner_class, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) @@ -174,7 +174,7 @@ def test_collection__is__contains__no_block__same_item_class first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element(previous_owners, 0, previous_owner_class, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) @@ -210,7 +210,7 @@ def test_collection__is__contains__no_block__different_item_class first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element(previous_owners, 0, previous_owner_class, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) @@ -244,7 +244,7 @@ def test_collection__no_is__contains__no_block first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element(previous_owners, 0, previous_owner_class, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) @@ -280,7 +280,7 @@ def test_collection__no_is__contains__block first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element_with_intermediary_class(previous_owners, 0, previous_owner_class, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) @@ -323,7 +323,7 @@ def test_collection__is__contains__block first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element_with_intermediary_class(previous_owners, 0, previous_owner_class, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) @@ -351,7 +351,7 @@ def test_collection__no_is__no_contains__no_block first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element(previous_owners, 0, AePageObjects::Element, first_owner_page_object) end @@ -376,7 +376,7 @@ def test_collection__locator first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element_with_intermediary_class(previous_owners, 0, AePageObjects::Element, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) @@ -411,7 +411,7 @@ def test_nested_element__locator__proc first_owner_page_object = stub(:allow_reload!) previous_owners_page_object.expects(:all).with(:xpath, ".//*", {}).returns([first_owner_page_object]) - previous_owners_page_object.expects(:first).with(:xpath, ".//*[1]", {}).returns(first_owner_page_object) + previous_owners_page_object.expects(:first).with(:xpath, "(.//*)[1]", {}).returns(first_owner_page_object) first_owner = verify_item_element_with_intermediary_class(previous_owners, 0, AePageObjects::Element, first_owner_page_object) owner_name_page_object = stub(:allow_reload!) From cb6980dd791f7c74b8b3e098e2488242ccb6ea64 Mon Sep 17 00:00:00 2001 From: Chris Horuk Date: Mon, 24 Jul 2017 17:40:10 -0700 Subject: [PATCH 2/2] ch - modify authors form book fields to highlight xpath selector bug - update form html to have an element in between the book field rows container and the individual book field row - update page object to not have item_locator that is direct descendant of collection locator --- .../shared/test/page_objects/authors/new_page.rb | 2 +- test/test_apps/shared/views/authors/_form.html.erb | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/test_apps/shared/test/page_objects/authors/new_page.rb b/test/test_apps/shared/test/page_objects/authors/new_page.rb index 68b4126d..e6db01a5 100644 --- a/test/test_apps/shared/test/page_objects/authors/new_page.rb +++ b/test/test_apps/shared/test/page_objects/authors/new_page.rb @@ -11,7 +11,7 @@ class NewPage < AePageObjects::Document collection :books, :name => "books_attributes", :locator => "#author_books", - :item_locator => ".some-books-fool .row" do + :item_locator => ".a-book-fool" do element :title end end diff --git a/test/test_apps/shared/views/authors/_form.html.erb b/test/test_apps/shared/views/authors/_form.html.erb index 7c14a58f..1fb31a90 100644 --- a/test/test_apps/shared/views/authors/_form.html.erb +++ b/test/test_apps/shared/views/authors/_form.html.erb @@ -20,7 +20,7 @@ Show Star Add Star Remove Star - + * @@ -41,17 +41,17 @@ <%= f.text_field :last_name %>
-
- <%= f.fields_for :books do |book_form| %> -
+ <%= f.fields_for :books do |book_form| %> +
+
<%= book_form.label :title %> <%= book_form.text_field :title %> <%= book_form.fields_for :index do |index_form| %> <%= index_form.text_field :pages %> <% end %>
- <% end %> -
+
+ <% end %>
<%= f.submit %>