Introduction of abstraction around @FindBy annotations and PageFactory

[paul-hammant]

• By placing an X in the preceding checkbox, I verify that I have signed the Contributor License Agreement

See the discussion of dev list "Extending @findby" (Mar 4th)

Talking through the diff:

1. AbstractFindByBuilder.java is new. It is really just the migration of methods from AbstractAnnotations.java though.

2. @annotations are not allowed to have parent annotations, interfaces or classes. That could change in Java10, as it seems arbitrary. Thus there's a new @PageFactoryFinder annotation that each implements now, and that one names a helper class that does the factory business. Of the pre-existing FindBys, each now has such a helper class (as a static inner class for no reason other than co-locating associated code).

3. Previous tests still pass, plus there's a test for a simulated/contrived alien @findby annotation.

In my opinion, this is OK to slot into a point revision of Selenium as it doesn't break backwards compatibility.

[shs96c]

First off, my sincere apologies this took so long. Inexcusable.

I've had a quick scan, and this looks good. A couple of questions:

1/ Why move all the logic out of AbstractAnnotations? People who are currently using this class are going to be in for a bit of a shock.

2/ The original idea with the PageFactory was that people would write their own FieldDecorators. I guess that approach is too complex? Put another way, how come this isn't implemented as a new FieldDecorator that allows for easier extension?

[paul-hammant]

I didn't think AbstractAnnotations could be reused outside the package - my bad. All that could go back again. I think the permissions on methods were only broadened in my change.

FieldDecorators? I'm not sure what that is. I think end-users wanted to be able to call a single PageFactory.initElements() and have all of the supplied @findxxx fields and all of the third-party
@findxxx fields handled.

[shs96c]

The PageFactory ultimately uses a FieldDecorator to shovel in java.lang.reflect.Proxy instances to fields it cares about. The default implementation looks for WebElement and List<WebElement> and then sticks in something that loads elements lazily on first look up, possibly using the @FindBy to figure out what to do. The Annotations stuff was originally broken out to make it easier for folks to write their own.

It seems that this is neither obvious nor easy to extend, which is why most of the requests we get are around improving the default behaviour. At least your PR moves that forward a little, which is a Good Thing.

Frankly, the whole PageFactory thing is a mess. If someone came along and actually cleaned it up to be easier to extend (particularly for custom types, such as Select or UI wrappers created by users) I'd be happy as a lark.

[shs96c]

Merging this in, though I think that buildIt could probably do with having the field passed in as well as the annotation itself (allowing locators to be inferred from the field name, or the FindBy implementation to use other annotations on the same field.

[shs96c]

Thanks, @paul-hammant! Merged as 0b30188

[paul-hammant]

:)