Repository navigation
feat(pagefactory): support custom @PageFactoryFinder locator annotations - #2499
hoanle0126 wants to merge 1 commit into
Conversation
5810b53 to
a0789c5
Compare
mykola-mokhnach
left a comment
mykola-mokhnach
left a comment
There was a problem hiding this comment.
-
[major] The PR ticks "New feature (non-breaking change)", but it changes how existing page objects behave, so it looks like a breaking change:
- A field or widget class that combines
@FindBy,@FindBysor@FindAll(Appium or Selenium) with another annotation carrying@PageFactoryFinderused to work, because onlyFindBy,FindBysandFindAllwere inspected. It now throwsIllegalArgumentExceptionfrominitElements(DefaultElementByBuilder.java:194). - A field annotated only with an existing Selenium
@PageFactoryFinderannotation used to fall back to the default locator (the field name). It is now resolved through the custom builder, so it returns different elements. DefaultFieldDecoratornow decoratesList<WebElement>fields it used to skip (DefaultFieldDecorator.java:110).
Either mark the PR as a breaking change (
feat!:, tick the box, describe the impact), or make the new resolution not affect page objects that work today. - A field or widget class that combines
-
[major] Revert the changes to
docs/openrewrite/migrate-to-v11.ymlanddocs/v10-to-v11-migration-guide.md. A non-breaking feature must not change the v10→v11 migration guides, and these edits contradict the "non-breaking" tick. If the change really needs migration notes, that confirms it is breaking. -
[minor]
docs/Page-objects.mddoesn't describe custom@PageFactoryFinderannotations. It should cover the builder contract, that the builder needs a no-arg constructor, thatbuildItgets anullfield for class-level annotations, and what happens when finders are combined.
| return field.getAnnotation(FindBy.class) != null | ||
| || field.getAnnotation(FindBys.class) != null | ||
| || field.getAnnotation(FindAll.class) != null; | ||
| return Annotations.hasLocatorAnnotation(field); |
There was a problem hiding this comment.
[major] hasLocatorAnnotation is now true for Selenium annotations and for any annotation carrying Selenium's @PageFactoryFinder. Before, only Appium @FindBy, @FindBys and @FindAll made a List<WebElement> field decoratable. A subclass of DefaultFieldDecorator with its own ElementLocatorFactory that doesn't understand those annotations now gets createLocator(field) for list fields it skipped before. This is a behavior change, so it's part of the breaking-change point in the review summary.
| defaultBy = new FindAll.FindByBuilder().buildIt(findAll, field); | ||
| } | ||
| List<Annotation> finders = resolveFinderAnnotations(annotatedElement); | ||
| validateFinderAnnotations(finders); |
There was a problem hiding this comment.
[major] Combinations that used to be accepted now throw. For example, @FindBy(id = "x") plus another annotation meta-annotated with @PageFactoryFinder (Appium or Selenium) used to work, because the second one was ignored. Now validateFinderAnnotations throws IllegalArgumentException for it. This is a breaking change for existing page objects. Please document and flag it, or limit the conflict check to combinations that already failed before.
| if (!declared.isEmpty() || !(annotatedElement instanceof Class<?>)) { | ||
| return declared; | ||
| } | ||
| return collectFinderAnnotations(annotatedElement.getAnnotations(), annotatedElement); |
There was a problem hiding this comment.
[minor] For classes, the inherited annotations are only consulted when the class declares no finder of its own. For example, a base widget has @Inherited @DataTestId("x") and a subclass declares only @FindBy(id = "y"). The inherited @DataTestId is silently ignored with no error, but the same two annotations on one field throw. Is that intended? If it is, it needs documenting. If not, resolve both declared and inherited annotations and run the same validation.
|
|
||
| @Nullable | ||
| @SuppressWarnings("unchecked") | ||
| private static By buildFromFinder(Annotation annotation, Field field) { |
There was a problem hiding this comment.
[minor] The finder-resolution logic exists three times: here, in DefaultElementByBuilder and in SeleniumAnnotationsCompat. Each copy has its own builder instantiation, InvocationTargetException unwrapping, rethrowUnchecked, findLegacyFinderMeta and SELENIUM_* constants. DefaultElementByBuilder.buildFromFinderAnnotation doesn't even use rethrowUnchecked. A fix in one copy won't reach the others. Annotations is not used anywhere in main, apart from hasLocatorAnnotation, so the support layer duplicates the pagefactory layer. Keep one implementation in support.pagefactory (collect finders / validate / build By) and have DefaultElementByBuilder and SeleniumAnnotationsCompat delegate to it, since pagefactory already depends on support.
| } | ||
|
|
||
| @Override | ||
| public By buildBy() { |
There was a problem hiding this comment.
[minor] buildBy() passes a Selenium @FindAll or @FindBys straight to Selenium's builder, which returns Selenium's ByAll or ByChained. DefaultElementByBuilder adapts the same annotation to the Appium FindAll/FindBys and returns io.appium.java_client.support.pagefactory.ByAll/ByChained. Identical source gives different By classes depending on the entry point, and this path needs selenium-support at runtime. Adapt the annotation the same way in both places, which is another reason to merge the two copies.
| @@ -0,0 +1,15 @@ | |||
| package io.appium.java_client.support; | |||
There was a problem hiding this comment.
[minor] The new main files PageFactoryFinder.java and Annotations.java have no Apache license header, unlike the other main sources (for example AbstractFindByBuilder.java, FindBy.java).
|
|
||
| /** Marks a custom locator annotation with its builder class. */ | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target({ElementType.TYPE, ElementType.ANNOTATION_TYPE}) |
There was a problem hiding this comment.
[nit] ElementType.TYPE already covers annotation types, so ANNOTATION_TYPE is redundant.
mykola-mokhnach
left a comment
mykola-mokhnach
left a comment
There was a problem hiding this comment.
in the current state this PR could only be merged as part of the next major release of java client
Proposed changes
Fixes #1043
@PageFactoryFinderandAnnotationstoio.appium.java_client.support, and attach finder metadata to the built-in FindBy, FindBys, and FindAll annotations.DefaultElementByBuilderandDefaultFieldDecoratorto resolve custom locator annotations extendingAbstractFindByBuilderacross single elements, element lists, widgets (both field-level and class-level), and By / AppiumBy fields.org.openqa.selenium.support.PageFactoryFindercustom annotations reflectively inSeleniumAnnotationsCompatwhen selenium-support is on the classpath, and include the new types in the v11 OpenRewrite migration recipe and guide.IllegalArgumentExceptionusing the declared annotation names.Types of changes