site-prism / site_prism

BSD 3-Clause "New" or "Revised" License
318 stars 40 forks source link

has_some_element? doesn't check for uniqueness #47

Open ineverov opened 4 years ago

ineverov commented 4 years ago

Issue to raise for SitePrism framework

supposed there is an element some_element defined as

element :some_element, 'li'

and code

if some_page.has_some_element?
  some_page.some_element
end

Above example will raise an error if li element is not unique on the line some_page.some_element : Capybara::Ambiguous Exception: Ambiguous match, found N elements matching visible css "li"

This is happening in https://github.com/site-prism/site_prism/blob/34a1ec938bee8c7cbd6dfba067d0747d65906c5e/lib/site_prism/page.rb#L113 there we check for a selector, but not providing count: 1 option for element.

This issues can be seen in various places there?(:some_element), all_there? etc will also return true for above example. But will fail if you try to interact with element

Environment

Expected Behavior

has_some_element? to return false (maybe with warning "found multiple elements")

Actual Behavior

has_some_element? returns true but some_element fails with above error

Proposed workaround

 module Fix                                                                                                                  
   def element(name, *find_args)                                                                                             
     hash_args = find_args.last                                                                                              
     hash_args.merge!(count: 1) unless hash_args.has_key?(:count)                                                            
     super(name, *find_args)                                                                                                 
   end                                                                                                                       
   def section(name, *find_args)                                                                                             
     hash_args = find_args.last                                                                                              
     hash_args.merge!(count: 1) unless hash_args.has_key?(:count)                                                            
     super(name, *find_args)                                                                                                 
   end                                                                                                                       
 end                                                                                                                         

 SitePrism::DSL::ClassMethods.prepend(Fix)
luke-hill commented 4 years ago

SO what do we think about the following business questions.

My opinions

ineverov commented 4 years ago

@luke-hill The problem I'm facing is the following: I have a page object (SomePage) with the expected elements defined. In the test case, I want to check if elements are all there before performing other asserts.

some_page = SomePage.new
expect(some_page).to be_all_there
# .... some steps ....

some_page.some_expected_ ambiguous_element.some_method

The first time I noticed this issue, I was like Crap I missed it in expected_elements. But then I realized all_there? returns true even though the element is ambiguous. I'd want my test to fail expect(some_page).to be_all_there check. Ideally with some verbose message (I'll raise another issue about it with a request to add RSpec matcher for all_there?). I agree about config option like ambiguous_check = true/false

luke-hill commented 4 years ago

So in solution we propose the following (For now in the 3.x branch).

1) A new config option. Which by default is set to false. When set to true. Any call to has_xxx? for section or element which has 2+ will fail. Will need a new custom exception writing. 2) A logging option. Which every time a call to has_xxx? is called for section or element returns a value of 2+ will log a WARN level message that the value found was not unique.

ineverov commented 4 years ago

@luke-hill sounds like a plan. I'll try to work on it anytime soon