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

Finalization of the Widget feature #669

Merged
merged 13 commits into from Jul 20, 2017

Conversation

Projects
None yet
4 participants
@TikhomirovSergey
Member

TikhomirovSergey commented 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)
@TikhomirovSergey

This comment has been minimized.

Show comment
Hide comment
@TikhomirovSergey

TikhomirovSergey Jul 12, 2017

Member

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

Member

TikhomirovSergey commented Jul 12, 2017

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

@mykola-mokhnach

This comment has been minimized.

Show comment
Hide comment
@mykola-mokhnach

mykola-mokhnach Jul 12, 2017

Contributor

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

Contributor

mykola-mokhnach commented Jul 12, 2017

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

@SrinivasanTarget SrinivasanTarget added this to the 5.0.0 milestone Jul 13, 2017

@TikhomirovSergey

This comment has been minimized.

Show comment
Hide comment
@TikhomirovSergey

TikhomirovSergey Jul 13, 2017

Member

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

Member

TikhomirovSergey commented Jul 13, 2017

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

@mykola-mokhnach

The review is still in progress...

Show outdated Hide outdated src/main/java/io/appium/java_client/pagefactory/OverrideWidgetReader.java
.orElse(getByFromDeclaredClass(WhatIsNeeded.DEFAULT_OR_HTML));
}
@Override protected By buildMobileNativeBy() {
return Optional.ofNullable(super.buildMobileNativeBy())
return ofNullable(super.buildMobileNativeBy())

This comment has been minimized.

@mykola-mokhnach

mykola-mokhnach Jul 14, 2017

Contributor

is the super.buildMobileNativeBy marked as Nullable?

@mykola-mokhnach

mykola-mokhnach Jul 14, 2017

Contributor

is the super.buildMobileNativeBy marked as Nullable?

}
@Override
public void get(String url) {

This comment has been minimized.

@mykola-mokhnach

mykola-mokhnach Jul 14, 2017

Contributor

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

@mykola-mokhnach

mykola-mokhnach Jul 14, 2017

Contributor

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

This comment has been minimized.

@TikhomirovSergey
@TikhomirovSergey
Show outdated Hide outdated ...io/appium/java_client/pagefactory_tests/widget/tests/AbstractWidget.java
Show outdated Hide outdated ...io/appium/java_client/pagefactory_tests/widget/tests/AbstractWidget.java
@Override
public String toString() {
return by.toString();

This comment has been minimized.

@mykola-mokhnach

mykola-mokhnach Jul 14, 2017

Contributor

can by be equal to null?

@mykola-mokhnach

mykola-mokhnach Jul 14, 2017

Contributor

can by be equal to null?

This comment has been minimized.

@TikhomirovSergey
@TikhomirovSergey

TikhomirovSergey Jul 19, 2017

Member

@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

Show outdated Hide outdated ...ava_client/pagefactory_tests/widget/tests/android/AndroidWidgetTest.java
Show outdated Hide outdated ...java_client/pagefactory_tests/widget/tests/combined/CombinedAppTest.java
.map(widget -> widget.getSelfReference().getClass()).collect(toList())));
assertThat(classes,
contains(widgetClass, widgetClass, widgetClass, widgetClass));

This comment has been minimized.

@mykola-mokhnach

mykola-mokhnach Jul 14, 2017

Contributor

can you please explain what is being verified here?

@mykola-mokhnach

mykola-mokhnach Jul 14, 2017

Contributor

can you please explain what is being verified here?

Show outdated Hide outdated ...ava/io/appium/java_client/pagefactory_tests/widget/tests/ios/IosApp.java
@TikhomirovSergey

This comment has been minimized.

Show comment
Hide comment
@TikhomirovSergey

TikhomirovSergey Jul 14, 2017

Member

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

Member

TikhomirovSergey commented Jul 14, 2017

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

The refactoring of widget tests: work on remarks and codecy issues
- comments for stub void methods
- method and classes were renamed.

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@appium appium deleted a comment from codacy-bot Jul 16, 2017

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Jul 19, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- src/main/java/io/appium/java_client/pagefactory/OverrideWidgetReader.java  1
- src/main/java/io/appium/java_client/pagefactory/utils/WebDriverUnpackUtility.java  2
         

See the complete overview on Codacy

codacy-bot commented Jul 19, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- src/main/java/io/appium/java_client/pagefactory/OverrideWidgetReader.java  1
- src/main/java/io/appium/java_client/pagefactory/utils/WebDriverUnpackUtility.java  2
         

See the complete overview on Codacy

@appium appium deleted a comment from codacy-bot Jul 19, 2017

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

3 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment