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

#764 alternative fix #769

Merged
merged 6 commits into from Nov 21, 2017

Conversation

Projects
None yet
5 participants
@TikhomirovSergey
Member

TikhomirovSergey commented Nov 17, 2017

Change list

#764 FIX. This change was made according to the confersation #765

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 added some commits Nov 14, 2017

#764 FIX.
The asking for the current platform was optimized.
#764 FIX.
The asking for the browser or not was optimized.
#764 FIX: DefaultGenericMobileDriver: toString method was changed
Also:
- WidgetListInterceptor was optimized
- some code style improvements
@TikhomirovSergey

This comment has been minimized.

Show comment
Hide comment
@TikhomirovSergey

TikhomirovSergey Nov 17, 2017

Member

@vrunoa You can review it too. It would be cool if you could try to test this change on your environment.

Member

TikhomirovSergey commented Nov 17, 2017

@vrunoa You can review it too. It would be cool if you could try to test this change on your environment.

ElementMap element = Optional.ofNullable(mobileElementMap.get(String
.valueOf(hasSessionDetails.getAutomationName()).toLowerCase().trim()))
ElementMap element = Optional.ofNullable(mobileElementMap.get(
String.valueOf(platform).toLowerCase().trim()))

This comment has been minimized.

@mykola-mokhnach

mykola-mokhnach Nov 18, 2017

Contributor

does it make any sense to use String.valueOf for values of type String?

@mykola-mokhnach

mykola-mokhnach Nov 18, 2017

Contributor

does it make any sense to use String.valueOf for values of type String?

This comment has been minimized.

@TikhomirovSergey

TikhomirovSergey Nov 18, 2017

Member

@mykola-mokhnach It may return null value

@TikhomirovSergey

TikhomirovSergey Nov 18, 2017

Member

@mykola-mokhnach It may return null value

@Override public WebElement findElement(SearchContext context) {
return context.findElement(map.get(getCurrentContentType(context)));
return context.findElement(map.get(currentContent));

This comment has been minimized.

@mykola-mokhnach

mykola-mokhnach Nov 18, 2017

Contributor

is it ok if map.get returns null in any of these methods?

@mykola-mokhnach

mykola-mokhnach Nov 18, 2017

Contributor

is it ok if map.get returns null in any of these methods?

This comment has been minimized.

@TikhomirovSergey

TikhomirovSergey Nov 18, 2017

Member

It is not possible. It will use NATIVE_MOBILE_SPECIFIC as it is defined by default or it will use HTML_OR_DEFAULT when there is no SearchContext which also implements ContextArare or HasSessionDetails. Also it will use HTML_OR_DEFAULT when HasSessionDetails.isBrowser returns true or current context != NATIVE_CONTEXT.

Also there is the method

    /**
     * This method sets required content type for the further searching.
     * @param type required content type {@link ContentType}
     * @return self-reference.
     */
    public By useContent(@Nonnull ContentType type) {
        checkNotNull(type);
        currentContent = type;
        return this;
    }
@TikhomirovSergey

TikhomirovSergey Nov 18, 2017

Member

It is not possible. It will use NATIVE_MOBILE_SPECIFIC as it is defined by default or it will use HTML_OR_DEFAULT when there is no SearchContext which also implements ContextArare or HasSessionDetails. Also it will use HTML_OR_DEFAULT when HasSessionDetails.isBrowser returns true or current context != NATIVE_CONTEXT.

Also there is the method

    /**
     * This method sets required content type for the further searching.
     * @param type required content type {@link ContentType}
     * @return self-reference.
     */
    public By useContent(@Nonnull ContentType type) {
        checkNotNull(type);
        currentContent = type;
        return this;
    }
@mykola-mokhnach

LGTM, just minor comments

@TikhomirovSergey

This comment has been minimized.

Show comment
Hide comment
@TikhomirovSergey

TikhomirovSergey Nov 18, 2017

Member

@mykola-mokhnach I have improved some things that you pointed.
@SrinivasanTarget I am waining for your review.

Member

TikhomirovSergey commented Nov 18, 2017

@mykola-mokhnach I have improved some things that you pointed.
@SrinivasanTarget I am waining for your review.

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

@SrinivasanTarget

This comment has been minimized.

Show comment
Hide comment
@SrinivasanTarget
Member

SrinivasanTarget commented Nov 19, 2017

LGTM

@TikhomirovSergey

This comment has been minimized.

Show comment
Hide comment
@TikhomirovSergey

TikhomirovSergey Nov 19, 2017

Member

@vrunoa
Also it is interesting to know about the time. Was it increased or reduced with this change on your environment? I will wait for a day or two days.

Member

TikhomirovSergey commented Nov 19, 2017

@vrunoa
Also it is interesting to know about the time. Was it increased or reduced with this change on your environment? I will wait for a day or two days.

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Nov 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/AppiumElementLocator.java  2
         

See the complete overview on Codacy

codacy-bot commented Nov 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/AppiumElementLocator.java  2
         

See the complete overview on Codacy

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

@vrunoa

This comment has been minimized.

Show comment
Hide comment
@vrunoa

vrunoa Nov 20, 2017

@TikhomirovSergey i'm running tests, i'll get back to you as soon as I have some results.

vrunoa commented Nov 20, 2017

@TikhomirovSergey i'm running tests, i'll get back to you as soon as I have some results.

@TikhomirovSergey

This comment has been minimized.

Show comment
Hide comment
@TikhomirovSergey

TikhomirovSergey Nov 20, 2017

Member

@vrunoa and what the result?

Member

TikhomirovSergey commented Nov 20, 2017

@vrunoa and what the result?

@TikhomirovSergey

This comment has been minimized.

Show comment
Hide comment
@TikhomirovSergey
Member

TikhomirovSergey commented Nov 21, 2017

ping @vrunoa

@vrunoa

This comment has been minimized.

Show comment
Hide comment
@vrunoa

vrunoa Nov 21, 2017

@TikhomirovSergey sorry for the delay. I'm session a lot less getSession calls and test times got reduced. Lets merge it.

vrunoa commented Nov 21, 2017

@TikhomirovSergey sorry for the delay. I'm session a lot less getSession calls and test times got reduced. Lets merge it.

@TikhomirovSergey

This comment has been minimized.

Show comment
Hide comment
@TikhomirovSergey

TikhomirovSergey Nov 21, 2017

Member

@vrunoa How much time was reduced? I will merge it later today

Member

TikhomirovSergey commented Nov 21, 2017

@vrunoa How much time was reduced? I will merge it later today

@vrunoa

This comment has been minimized.

Show comment
Hide comment
@vrunoa

vrunoa Nov 21, 2017

@TikhomirovSergey from 2hs to 50minutes approximately.

vrunoa commented Nov 21, 2017

@TikhomirovSergey from 2hs to 50minutes approximately.

@SrinivasanTarget

This comment has been minimized.

Show comment
Hide comment
@SrinivasanTarget

SrinivasanTarget Nov 21, 2017

Member

2hs to 50minutes approximately

Awesome

Member

SrinivasanTarget commented Nov 21, 2017

2hs to 50minutes approximately

Awesome

@TikhomirovSergey TikhomirovSergey merged commit dd8f33e into appium:master Nov 21, 2017

2 checks passed

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