Finalization of the Widget feature by TikhomirovSergey · Pull Request #669 · appium/java-client

TikhomirovSergey · Jul 12, 2017

Change list

missed parameters of the OverrideWidget were added
- iOSXCUITAutomation
- windowsAutomation
some improvements which are related to latest API changes were made.
tests were refactored. Now it is supposed to use unit test for this feature

Types of changes

No changes in production code.
Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

…t were added also old tests were removed

…get are handled now

…essionDetails#isBrowser now and the order of imports was improved

…app was added ...also some improvements

- all test were added - checkstyle issues were fixed

TikhomirovSergey · Jul 12, 2017

@SrinivasanTarget @mykola-mokhnach could you take a look at this PR?

mykola-mokhnach · Jul 12, 2017

It's huge O_O. Probably, this will take some time

TikhomirovSergey · Jul 13, 2017

@mykola-mokhnach. Ok. Don't hurry and be watchful.

mykola-mokhnach

The review is still in progress...

mykola-mokhnach · Jul 14, 2017

src/main/java/io/appium/java_client/pagefactory/OverrideWidgetReader.java

            return getConvenientClass(declaredClass, annotatedElement, ANDROID_UI_AUTOMATOR);
        }

+        if (IOS.equalsIgnoreCase(transformedPlatform) && AutomationName.IOS_XCUI_TEST


I'd rather added this condition under single IOS.equalsIgnoreCase(transformedPlatform) condition. One might forget about the fact, that conditions order matters here, having the current implementation.

mykola-mokhnach · Jul 14, 2017

src/main/java/io/appium/java_client/pagefactory/WidgetByBuilder.java


    @Override protected By buildMobileNativeBy() {
-        return Optional.ofNullable(super.buildMobileNativeBy())
+        return ofNullable(super.buildMobileNativeBy())


is the super.buildMobileNativeBy marked as Nullable?

mykola-mokhnach · Jul 14, 2017

src/test/java/io/appium/java_client/pagefactory_tests/widget/tests/AbstractStubWebDriver.java

+    }
+
+    @Override
+    public void get(String url) {


probably, such empty method tucbs should also have some comment in the body to make linter happy

@mykola-mokhnach FIXED

mykola-mokhnach · Jul 14, 2017

src/test/java/io/appium/java_client/pagefactory_tests/widget/tests/AbstractWidget.java

+    }
+
+    public <T extends Widget> List<T> getSubWidgets() {
+        return of();


I cannot say this is very readable %)

mykola-mokhnach · Jul 14, 2017

src/test/java/io/appium/java_client/pagefactory_tests/widget/tests/AbstractWidget.java

+    }
+
+    public String toString() {
+        return getWrappedElement().toString();


can getWrappedElement return null?

mykola-mokhnach · Jul 14, 2017

src/test/java/io/appium/java_client/pagefactory_tests/widget/tests/StubWebElement.java

+
+    @Override
+    public String toString() {
+        return by.toString();


can by be equal to null?

@mykola-mokhnach I have fixed the code. Please take a look at the
https://github.com/appium/java-client/pull/669/files/0db6a3093c6bb64f07e4f85c907da22452c293b1#diff-8d9d46cd20efb5a5c8a11fef4fc8e5e4R22

Also this situation is not possible in production code

mykola-mokhnach · Jul 14, 2017

...est/java/io/appium/java_client/pagefactory_tests/widget/tests/android/AndroidWidgetTest.java

+
+    @Override
+    public void checkACommonWidget() {
+        defaultTest(app.getWidget(), app.getWidgets(),


can we have a better name for this method?

mykola-mokhnach · Jul 14, 2017

...test/java/io/appium/java_client/pagefactory_tests/widget/tests/combined/CombinedAppTest.java

+     */
+    @Parameterized.Parameters
+    public static Collection<Object[]> data() {
+        return asList(new Object[][] {


nit: Arrays.asList also supports simple objects (T... item) as parameters

mykola-mokhnach · Jul 14, 2017

...t/java/io/appium/java_client/pagefactory_tests/widget/tests/combined/CombinedWidgetTest.java

+                        .map(widget -> widget.getSelfReference().getClass()).collect(toList())));
+
+        assertThat(classes,
+                contains(widgetClass, widgetClass, widgetClass, widgetClass));


can you please explain what is being verified here?

mykola-mokhnach · Jul 14, 2017

src/test/java/io/appium/java_client/pagefactory_tests/widget/tests/ios/IosApp.java

+    @iOSXCUITFindBy(iOSNsPredicate = "SOME_XCUIT_DEFAULT_LOCATOR")
+    private List<DefaultIosWidget> multipleIosWidgets;
+
+    private AnnotatedIosWidget singleAnnotatedIosWidget;


should these properties be annotated as well?

mykola-mokhnach · Jul 14, 2017

.../java/io/appium/java_client/pagefactory_tests/widget/tests/windows/DefaultWindowsWidget.java

+
+    public static String WINDOWS_SUB_WIDGET_LOCATOR = "SOME_SUB_LOCATOR";
+
+    @WindowsFindBy(windowsAutomation =  "SOME_SUB_LOCATOR")


nit: format

mykola-mokhnach · Jul 14, 2017

src/test/java/io/appium/java_client/pagefactory_tests/widget/tests/windows/WindowsApp.java

+    @iOSXCUITFindBy(iOSNsPredicate = "SOME_XCUIT_DEFAULT_LOCATOR")
+    private List<DefaultWindowsWidget> multipleIosWidgets;
+
+    private AnnotatedWindowsWidget singleAnnotatedIosWidget;


missing annotations?

mykola-mokhnach · Jul 14, 2017

src/test/java/io/appium/java_client/pagefactory_tests/widget/tests/AbstractStubWebDriver.java

+            return APPIUM;
+        }
+
+        public boolean isBrowser() {


TikhomirovSergey · Jul 14, 2017

@mykola-mokhnach Ok. I'm working on you remarks.

- comments for stub void methods - method and classes were renamed.

mykola-mokhnach · Jul 17, 2017

...est/java/io/appium/java_client/pagefactory_tests/widget/tests/android/AndroidWidgetTest.java

-    public void checkACommonWidget() {
-        defaultTest(app.getWidget(), app.getWidgets(),
+    public void commonTestCase() {
+        testLogigByDefault(app.getWidget(), app.getWidgets(),


what is "Logig"?

It is typo :)

mykola-mokhnach · Jul 17, 2017

src/test/java/io/appium/java_client/pagefactory_tests/widget/tests/WidgetTest.java


    @Test
-    public abstract void checkACommonWidget();
+    public abstract void commonTestCase();


it is recommended having a verb in method names

mykola-mokhnach · Jul 17, 2017

src/test/java/io/appium/java_client/pagefactory_tests/widget/tests/ios/IosWidgetTest.java

    @Override
-    public void checkAnAnnotatedWidget() {
-        defaultTest(((ExtendedApp) app).getAnnotatedWidget(), ((ExtendedApp) app).getAnnotatedWidgets(),
+    public void checkCaseWhenWidgetClassHasDeclaredLocatorAnnotation() {


TikhomirovSergey added 11 commits June 24, 2017 23:49

The refactoring of widget tests: The beginning

da7a1a6

The refactoring of widget tests: Stub webdriver/webelement

162fe15

The refactoring of widget tests: the WidgetTest interface

282fd34

The refactoring of widget tests: Android widget tests were added

1e06afe

The refactoring of widget tests: All simple cases were implemented

a079f48

The refactoring of widget tests: lost parameters of the OverrideWidge…

1ceec6a

…t were added also old tests were removed

The refactoring of widget tests: missed parameters of the OverrideWid…

4c92e82

…get are handled now

The refactoring of widget tests: WebDriverUnpackUtility is using HasS…

30d0163

…essionDetails#isBrowser now and the order of imports was improved

The refactoring of widget tests: Test API was re-designed

96ab7fd

The refactoring of widget tests: the test with fully combined widget …

5c960a4

…app was added ...also some improvements

The refactoring of widget tests: The final commit

0db6a30

- all test were added - checkstyle issues were fixed

TikhomirovSergey requested review from SrinivasanTarget and mykola-mokhnach July 12, 2017 21:19

TikhomirovSergey self-assigned this Jul 12, 2017

SrinivasanTarget added this to the 5.0.0 milestone Jul 13, 2017

TikhomirovSergey added enhancement testing Windows iOS labels Jul 13, 2017

mykola-mokhnach reviewed Jul 14, 2017

View reviewed changes

The refactoring of widget tests: work on remarks and codecy issues

73589c8

- comments for stub void methods - method and classes were renamed.

appium deleted a comment Jul 16, 2017

mykola-mokhnach reviewed Jul 17, 2017

View reviewed changes

The refactoring of widget tests: work on remarks by @mykola-mokhnach

94660c7

mykola-mokhnach approved these changes Jul 19, 2017

View reviewed changes

appium deleted a comment Jul 19, 2017

SrinivasanTarget approved these changes Jul 20, 2017

View reviewed changes

TikhomirovSergey merged commit 8340d5c into appium:master Jul 20, 2017


		public static String WINDOWS_SUB_WIDGET_LOCATOR = "SOME_SUB_LOCATOR";

		@WindowsFindBy(windowsAutomation = "SOME_SUB_LOCATOR")

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

TikhomirovSergey commented Jul 12, 2017

Change list

Types of changes

Uh oh!

TikhomirovSergey commented Jul 12, 2017

Uh oh!

mykola-mokhnach commented Jul 12, 2017

Uh oh!

TikhomirovSergey commented Jul 13, 2017

Uh oh!

mykola-mokhnach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey commented Jul 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mykola-mokhnach Jul 14, 2017 •

edited

Loading

TikhomirovSergey Jul 19, 2017 •

edited

Loading