Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
aaltat opened this issue Oct 31, 2017 · 17 comments

Comments

@aaltat
Copy link
Contributor

@aaltat aaltat commented Oct 31, 2017

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 aaltat added this to the v3.1.0 milestone Oct 31, 2017
@aaltat aaltat changed the title If there are multiple elements found by locator then keyword should fail If there are multiple elements found by locator then keyword should display warning in release 3.1 and release 3.2 keywords should fail. Oct 31, 2017
@aaltat aaltat changed the title If there are multiple elements found by locator then keyword should display warning in release 3.1 and release 3.2 keywords should fail. Deprecate selecting silently the first element when multiple elements are found by the locator. Oct 31, 2017
@aaltat aaltat added the RoboCon label Jan 16, 2018
@aaltat aaltat removed the RoboCon label Jan 30, 2018
@aaltat aaltat modified the milestones: v3.2.0, v3.3.0 Mar 6, 2018
@rubygeek
Copy link
Contributor

@rubygeek rubygeek commented Apr 12, 2018

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

@rubygeek
Copy link
Contributor

@rubygeek rubygeek commented Apr 13, 2018

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.

@pekkaklarck
Copy link
Member

@pekkaklarck pekkaklarck commented Apr 13, 2018

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.

@rubygeek
Copy link
Contributor

@rubygeek rubygeek commented Apr 13, 2018

@aaltat
Copy link
Contributor Author

@aaltat aaltat commented Apr 13, 2018

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.

@pekkaklarck
Copy link
Member

@pekkaklarck pekkaklarck commented Apr 14, 2018

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.

@emanlove
Copy link
Contributor

@emanlove emanlove commented Apr 16, 2018

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

@aaltat
Copy link
Contributor Author

@aaltat aaltat commented Apr 17, 2018

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.

@emanlove
Copy link
Contributor

@emanlove emanlove commented Apr 18, 2018

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..

@rubygeek
Copy link
Contributor

@rubygeek rubygeek commented Apr 19, 2018

@aaltat aaltat self-assigned this Jun 22, 2018
@aaltat aaltat changed the title Deprecate selecting silently the first element when multiple elements are found by the locator. When multiple elements are found by the locator and keyword interacts with single element, generate a warning in the logs Jun 30, 2018
@aaltat
Copy link
Contributor Author

@aaltat aaltat commented Jun 30, 2018

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.

@pekkaklarck
Copy link
Member

@pekkaklarck pekkaklarck commented Jul 3, 2018

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." % (...))
@aaltat
Copy link
Contributor Author

@aaltat aaltat commented Jul 4, 2018

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.

@pekkaklarck
Copy link
Member

@pekkaklarck pekkaklarck commented Jul 4, 2018

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.

@rubygeek
Copy link
Contributor

@rubygeek rubygeek commented Jul 4, 2018

@aaltat
Copy link
Contributor Author

@aaltat aaltat commented Jul 5, 2018

@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 :-)

@pekkaklarck
Copy link
Member

@pekkaklarck pekkaklarck commented Jul 5, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.