Skip to content
This repository has been archived by the owner on Nov 29, 2018. It is now read-only.

PageFactory incorrectly decorates all List fields #2674

Closed
lukeis opened this issue Mar 3, 2016 · 18 comments
Closed

PageFactory incorrectly decorates all List fields #2674

lukeis opened this issue Mar 3, 2016 · 18 comments
Assignees
@lukeis
Labels
Component-WebDriver Lang-Java Priority-Medium Restrict-AddIssueComment-Commit Status-Fixed Type-Defect

Comments

@lukeis
Copy link
Member

lukeis commented Mar 3, 2016

Originally reported on Google Code with ID 2674

As of 2.8.0, PageFactory.initElements() now decorates Lists as well as WebElements.
 (http://code.google.com/p/selenium/source/detail?r=14014)

However, the change is too broad, and affects all Lists, not just Lists of WebElements.

The following test works in 2.7.0, but fails in 2.8.0:

    @Test
    public void canHaveListOfStringsInPageObject() {
        MyPageObject myPageObject = PageFactory.initElements(new HtmlUnitDriver(),
MyPageObject.class);
        assertEquals("string1", myPageObject.strings.get(0));
    }

    public static class MyPageObject {
        List<String> strings = Arrays.asList("string1", "string2");
    }

with the exception:

java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
    at java.util.ArrayList.RangeCheck(ArrayList.java:547)
    at java.util.ArrayList.get(ArrayList.java:322)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.openqa.selenium.support.pagefactory.internal.LocatingElementListHandler.invoke(LocatingElementListHandler.java:39)
    at $Proxy4.get(Unknown Source)
    at test.PageObjectTest.canHaveListOfStringsInPageObject(PageObjectTest.java:16)

This has broken some of our existing tests, which hold a list of table headers as strings.

Due to type erasure, I don't think you'll be able to check the list element type at
runtime. My suggestion is to only decorate Lists if they are annotated with @FindBy.

Reported by nigel.charman.nz on 2011-10-17 08:02:56
@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

Looking back through issue 417, comment 6 links to http://code.google.com/p/selenium/issues/detail?id=1602,
which has a suggestion for how to maintain reification through extension.

Reported by nigel.charman.nz on 2011-10-17 08:13:19

@lukeis lukeis self-assigned this Mar 3, 2016
@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016

Reported by barancev on 2011-10-17 10:13:45

  • Labels added: Lang-Java, Component-WebDriver

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

I really hope I'm wrong here, but it looks like PageFactory/DefaultElementLocator/DefaultFieldDecorator
don't actually look to see if an annotation is present on the field before decorating
it.

If that's the case, then I guess my only question is "Why?"  It seems like a dangerous
proposition to just blindly assign values, particularly to a generified type as is
reported in this ticket.  If you made sure the field was annotated first, this issue
would be limited to user error.

Reported by ian.g.simpson on 2011-10-17 23:00:13

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

You're not wrong. An annotation doesn't need to be present.  For WebElement fields with
no @FindBy annotation, the PageFactory will attempt to find an element with the id
or name matching the field name.  This is explained in http://code.google.com/p/selenium/wiki/PageFactory.

While this is a convenience for WebElements, I'm not sure it makes any sense for Lists.
The id attribute should be unique, so wouldn't be useful in a List situation.  

I agree that List fields should only be decorated when annotated with @FindBy.

Reported by nigel.charman.nz on 2011-10-17 23:23:06

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

I don't think it's necessarily a good thing for WebElement instances either, as it prevents
you from doing any kind of lazy loading or null checking within your page object. 


I think the convenience of not having to specify the criteria for finding the element
is valuable, but I think it should at least warrant adding @FindBy so that the end
user has control (and so that it's more obvious to people new to Selenium 2).  From
what I can gather you can't even mark the field as transient to avoid it being assigned
to something.

Reported by ian.g.simpson on 2011-10-18 00:13:16

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

We've also ran into this issue, and it is a major hurdle for us.  Failing hundreds of
our PageObjects/Tests impeding our upgrade from 2.7 to 2.9.  (and we desperately need
some of the 2.8 bug fixes).

I like comment 4 proposal, where List fields should be explicitly annotated.  (Also
agree with comment 5, as we also do some lazy/null post processing on PageObjects,
but we now have the code to handle that ok).

Reported by jason@jasonkissinger.com on 2011-10-25 18:18:29

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

Yes, I have to agree. Default location strategy "by id or name" that is quite reasonable
for WebElement fields is hardly valuable for List<WebElement> fields as there should
be a single element with a given id, and usually only one element has a given name.
Fixed by revision r14400. Now PageFactory should not decorate List<WebElement> fields
that have no @FindBy annotation.

Reported by barancev on 2011-10-27 12:08:36

  • Status changed: Fixed

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

Do you think I should open up another ticket for the default decoration of WebElement
fields or is that a discussion that should happen on the selenium-developers list?
 I still am of the mindset that those should not be decorated by default either.

Reported by ian.g.simpson on 2011-10-28 15:39:43

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

Yes, selenium-developers list is a better place to discuss design decisions.

Reported by barancev on 2011-10-28 16:09:25

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

so does anyone know when the FindAllBy support will be added back in?

Reported by pjmagee2 on 2011-11-07 11:22:34

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

It has been already added, you may use the same @FindBy and @FindBys annotations for
List<WebElement> fields.

Аnnotation @FindBys for lists was off in version 2.11 by mistake, it will be available
again in 2.12

Reported by barancev on 2011-11-07 11:37:09

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

One of our devs has found that List<WebElement> fields with no @FindBy annotation are
now being decorated.  This is contrary to comment 7, and has broken our tests that
already had a List<WebElement> field that was expected to be null.

Is this the expected behaviour?  There is no mention of it in the release notes?

Reported by nigel.charman.nz on 2011-12-04 21:49:19

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

This was not mentioned in the release notes because this is the expected behaviour (and
always has been what the WebElement decorator did). 

The idea of the PageFactory is to reduce the amount of boilerplate code in your tests.
Following the approach of "convention over configuration", if the field is named after
the element name or id, then we'll use that to look up the element(s). If it's missing,
or doesn't match, then the annotation allows you to override this behaviour.

Reported by simon.m.stewart on 2011-12-06 15:27:29

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

Thanks for clarifying this.

This certainly wasn't our expected behaviour, given that the feature was implemented
in 2.8.0 without implicit wrapping of unannotated fields.

On a related note, the PageFactory wiki page could do with an update.  It does not
mention the List<WebElement> behaviour.  And it still refers to RenderedWebElement.
 Should I open a separate issue for this?

Reported by nigel.charman@assurity.co.nz on 2011-12-06 18:13:53

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

I've updated the wiki page to document the new behavior and remove the references to
RenderedWebElement. Thanks for pointing it out!

Reported by simon.m.stewart on 2011-12-07 11:58:46

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

Well, it was rolled back due to miscommunication. I'll implement it again, and document
it better.

Reported by barancev on 2011-12-12 19:13:21

  • Status changed: Accepted

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016 

Fixed again by r15171

Reported by barancev on 2011-12-12 20:26:06

  • Status changed: Fixed

@lukeis
Copy link
Member Author

lukeis commented Mar 3, 2016

Reported by luke.semerau on 2015-09-17 18:13:58

  • Labels added: Restrict-AddIssueComment-Commit

@lukeis lukeis closed this as completed Mar 3, 2016
@SeleniumHQ SeleniumHQ locked and limited conversation to collaborators Mar 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@lukeis lukeis

Labels
Component-WebDriver Lang-Java Priority-Medium Restrict-AddIssueComment-Commit Status-Fixed Type-Defect
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@lukeis