SeleniumLibrary icon indicating copy to clipboard operation
SeleniumLibrary copied to clipboard

When multiple elements are found by the locator and keyword interacts with single element, generate a warning in the logs

Open aaltat opened this issue 8 years ago • 17 comments

Now if there are multiple elements found by the locator, then SL silently takes the first elements. This is not good implementation and we should want users about multiple elements found by the locators. We should perhaps display warning in the console and in the log.html that multiple elements. are found by the locator. Making this change is too late for 3.0 release.

Display warning in release 3.2.0. Should fail in release 3.3.0 (remember to create separate issue about failure when warning is done)

aaltat avatar Oct 31 '17 12:10 aaltat

If there are no objections, I am going to see if I can tackle this one :)

rubygeek avatar Apr 12 '18 17:04 rubygeek

I was trying to think of why having multiple items would be bad, and i guess only in the case of multiple id=[value] because id's are suppose to be unique within the document.

rubygeek avatar Apr 13 '18 02:04 rubygeek

It's mostly that it feels a bit random to just select the first element if there are multiple matches. And test automation should not be random. You should be able to construct a locator that matches on the desired element.

The above said, this change is backwards incompatible and may cause lot of tests to break. If that happens, we need to think is being pedantic that important. Practicality vs. purity once again.

pekkaklarck avatar Apr 13 '18 10:04 pekkaklarck

if i want multiple matches i use: css=.feature-article . if i want only first one i can use css=.feature-article:first-of-type ... if i want the second .feature-article i can use css=.feature-article:nth-child(2)

On Fri, Apr 13, 2018 at 5:17 AM, Pekka Klärck [email protected] wrote:

It's mostly that it feels a bit random to just select the first element if there are multiple matches. And test automation should not be random. You should be able to construct a locator that matches on the desired element.

The above said, this change is backwards incompatible and may cause lot of tests to break. If that happens, we need to think is being pedantic that important. Practicality vs. purity once again.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/robotframework/SeleniumLibrary/issues/978#issuecomment-381092237, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAoXVytaNDU__z_eRs4D5w7z6LohNQMks5toHtOgaJpZM4QMqmX .

-- Ruby: http://blog.rubygeek.com - http://www.twitter.com/rubygeek http://www.linkedin.com/in/nolastowe - my linkedin profile http://github.com/rubygeek - my code

rubygeek avatar Apr 13 '18 13:04 rubygeek

Yes that is possible and there are many other ways too. Also users that are skilful enough, do know how to select the correct single element. But when user is new to UI and SeleniumLibrary automation this can be a problem for the users. Also UI may change and once unique selector can now point to multiple elements. In those cases it would be good to receive feedback.

aaltat avatar Apr 13 '18 13:04 aaltat

Exactly, there should always be a way to pinpoint a single element. The library automatically discarding other than the first element can lead to hard to diagnose problems.

Another alternatively would be somehow acting on all found elements similarly as, for example, jQuery. I don't think that makes any sense in SL case, though.

pekkaklarck avatar Apr 14 '18 05:04 pekkaklarck

Why would/should the library fail when multiple elements are found as compared to giving a clear and informative warning?

emanlove avatar Apr 16 '18 12:04 emanlove

That was the initial idea and it sounded logical to me. When I think of it, I can see the failure would beneficial for the users that are not so familiar with browser UI automation. But I also see that more experienced user could be offended.

After the warning is implemented and released, we should ask from the community what they would like to happen.

aaltat avatar Apr 17 '18 13:04 aaltat

I ask mostly because I am just trying to work out in my mind what I would expect the behavior to be or how the system should react in such a situation. As I recall the internal workings of the keywords where a locator is passed in if only one element is expected like Click Element then I could an error when locator turns up multiple elements. My concern would be areas where it is ambiguous as to whether or not multiple elements is fine or places where we would, for whatever reason, need to understand the locator is perfectly fine to have multiple elements. I was also trying to think long term to see what are the possibilities for how we would handle locators or elements and would the behavior box us into limited solutions, or open us up to a better framework for using locators, or may have no affect..

emanlove avatar Apr 18 '18 23:04 emanlove

one of the parameters is first_only ...so wonder if we can make another function where that is passed in as False

https://github.com/rubygeek/SeleniumLibrary/blob/master/src/SeleniumLibrary/locators/elementfinder.py#L61

On Wed, Apr 18, 2018 at 6:48 PM, Ed Manlove [email protected] wrote:

I ask mostly because I am just trying to work out in my mind what I would expect the behavior to be or how the system should react in such a situation. As I recall the internal workings of the keywords where a locator is passed in if only one element is expected like Click Element then I could an error when locator turns up multiple elements. My concern would be areas where it is ambiguous as to whether or not multiple elements is fine or places where we would, for whatever reason, need to understand the locator is perfectly fine to have multiple elements. I was also trying to think long term to see what are the possibilities for how we would handle locators or elements and would the behavior box us into limited solutions, or open us up to a better framework for using locators, or may have no affect..

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/robotframework/SeleniumLibrary/issues/978#issuecomment-382564042, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAoXWc3P4iM-n5Hd9kZCZEi2HBxWWPUks5tp9DKgaJpZM4QMqmX .

-- Ruby: http://blog.rubygeek.com - http://www.twitter.com/rubygeek http://www.linkedin.com/in/nolastowe - my linkedin profile http://github.com/rubygeek - my code

rubygeek avatar Apr 19 '18 01:04 rubygeek

I have fix proposal in #1150 but I would need feedback on few items

  1. Page Should Contain keyword Now I did made that Page Should (Not) Contain does not log warning because it finds often multiple elements (Because of the xpath it internal creates) and with this keyword user is not interested about elements. User is interested about text in the page.

  2. Page Should Contain ... keywords Now, should other Page Should Contain ... keywords log the warning or not. I am thinking that they should, but I can think in other direction too. The complicated part is the Page Should Contain Radio Button keyword, because html could look like this:

<input type="radio" name="sex" value="female" checked /> Female<br/>
<input type="radio" name="sex" value="male" /> Male

And it could be used like this: | Page Should Contain Radio Button | sex |, which would lead to warning. But should it, because radio button could be seen as a single element (although it is not) and user want s to verify that radio button group is there.

aaltat avatar Jun 30 '18 22:06 aaltat

IMHO Contain keywords should generally work without warnings if there are multiple matches. For example, BuildIn Should Contain works that way. Implementation could be something like

elements = self.find_elements(...)
if not elements:
    raise AssertionError
self.info("%d element%s matched." % (...))

pekkaklarck avatar Jul 03 '18 16:07 pekkaklarck

It is good idea, but I honestly do not know yet (mostly because JavaScript argument thing has taken all the available time). I can think of exception to that rule: Page Should Contain Element, because in that we are interested about element(s). And what about Page Should Contain Button. Perhaps before implementing anything more, I need list all the keywords and decide what will and what will not log the warning. Then it is more meaningful to have a discussion, when full picture is available.

In any case, regardless to which direction we go, I think documenting this thing will be painful. The solution for the documentation, is to simple list all the keywords which will and which one will not log an warning when multiple elements are found.

Also it might be good idea to require testing this feature on new keywords. Also that should documented too.

aaltat avatar Jul 04 '18 19:07 aaltat

I don't see why Page Should Contain Element, Page Should Contain Button, or any other Contain keyword should fail or even emit a warning if there is more than one element, button, whatever. I prefer that approach in general, but now that we need to think about backwards compatibility failures or warnings would be especially problematic. Also, all/most of these keywords have limit=None argument that can be used to validate that there is a certain exact number of matches.

The actual problem this issue tries to address is that currently keywords doing something automatically select the first match. Just ignoring possible other matches is not good. I don't think keywords validating something (i.e. Should keywords) should be affected in general.

pekkaklarck avatar Jul 04 '18 20:07 pekkaklarck

Originally I started to take on this ticket but then realized it wouldn't be good. I've been following all along with the discussion but what would we gain by emiting warnings? even if it is invalid HTML, that is still not the purpose of RF. I can't really see a good reason to do this :(

On Wed, Jul 4, 2018 at 3:34 PM Pekka Klärck [email protected] wrote:

I don't see why Page Should Contain Element, Page Should Contain Button, or any other Contain keyword should fail or even emit a warning if there is more than one element, button, whatever. I prefer that approach in general, but now that we need to think about backwards compatibility failures or warnings would be especially problematic. Also, all/most of these keywords have limit=None argument that can be used to validate that there is a certain exact number of matches.

The actual problem this issue tries to address is that currently keywords doing something automatically select the first match. Just ignoring possible other matches is not good. I don't think keywords validating something (i.e. Should keywords) should be affected in general.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/robotframework/SeleniumLibrary/issues/978#issuecomment-402555456, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAoXU9zCaMC4fcNerRVf6ySozrQiH-Sks5uDSbugaJpZM4QMqmX .

-- Ruby: http://blog.rubygeek.com - http://www.twitter.com/rubygeek http://www.linkedin.com/in/nolastowe - my linkedin profile http://github.com/rubygeek - my code

rubygeek avatar Jul 04 '18 22:07 rubygeek

@pekkaklarck So the keywords that do not interact with the element, should not log a warning, like the Page Should Contain... and Wait Until Page... keywords. But keywords that interact with the element, like Click Button and Get Element Attribute should log warning if multiple elements are found with the locator. Is that the idea in nutshell? If it's then I can see the reasoning in your idea. Just need to give it a some tough.

@rubygeek The HTML is not invalid in the page. But the if the locator provided by the user matches to multiple elements, then the SeleniumLibrary will silently select the first element and performs the possible interact with the first element. And we are trying to solve part that instead being silent, the library would inform users about the decision it has made. The best idea so far has been to display a warning to users in the log.html file. But I am open to other ideas too and happy to change the implementation too.

I am sorry, that I did not have more time to spend with you when you started to implement this feature and you did implement the wrong thing. And it's not your fault at all, it's just different expectations what the feature should actually do. As you can see, I and Pekka have different expectations and we did actually discuss this face to face few times :-)

aaltat avatar Jul 05 '18 06:07 aaltat

As I've commented earlier, the problem is that there are cases when a locator matches multiple elements but SL just throws other than the first one away. The place where it happens is in ElementFinder.find(). This feels pretty random for me and test automation shouldn't be random. It ought to always be possible to construct a locator that matches exactly the desired element. For example, instead of using //h1 you ought to be able to use //h1[1].

This mostly affects keywords interacting with elements. For example, if I say Click Link class:xxx and there are multiple links with class xxx, blindly clicking the first link sounds wrong. Notice that jQuery would click all links in this case, but I don't think that's a good idea with SL. Requiring users to give an exact locator sounds much better.

I was wrong above when I validation keywords wouldn't be affected in general. It's true that Contains keyword shouldn't be affected, but keywords like Element Text Should Be probably are. My guess is that at the moment they also only check the first element and ignore others. I can see many ways this could create false positive results.

The main problem with the change is that it is backwards incompatible and likely requires many users to updated their locators. Although updating should generally be pretty easy, there may be lot of locators to update making the change problematic. We definitely cannot make the situation error right a way but emitting a warning ought to be fine. If there a lot of complaints, the warning can then be removed.

pekkaklarck avatar Jul 05 '18 10:07 pekkaklarck