ByAll was re-implemented. by HlebHalkouski · Pull Request #680 · appium/java-client

HlebHalkouski · Jul 28, 2017

Change list

ByAll was re-implemented. Now it return the fist founded element for single search.

Types of changes

What types of changes are you proposing/introducing to Java client?
Put an x in the boxes that apply

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)

Details

This changes improve time of seaching single element for 'All Possible' strategy. Now it doesn't search all possible elements for single.
Selenium implementation was for single search: invoke findElements method and return the first element in list.

…or single search.

jsf-clabot · Jul 28, 2017

All committers have signed the CLA.

mykola-mokhnach · Jul 28, 2017

src/main/java/io/appium/java_client/pagefactory/bys/builder/ByAll.java

+    public ByAll(By[] bys) {
+        super(bys);
+        checkNotNull(bys);
+        if (bys.length == 0) {


better to use isEmpty()

mykola-mokhnach · Jul 28, 2017

src/main/java/io/appium/java_client/pagefactory/bys/builder/ByAll.java

+    private By[] bys;
+
+    private Function<SearchContext, WebElement> getSearchingFunction(By by) {
+        return input -> {


return (input) -> ... is more readable

mykola-mokhnach · Jul 28, 2017

src/main/java/io/appium/java_client/pagefactory/bys/builder/ByAll.java

+
+    private By[] bys;
+
+    private Function<SearchContext, WebElement> getSearchingFunction(By by) {


I'd mark it with Nullable annotation

Or it would be even better to return an Optional value

mykola-mokhnach · Jul 28, 2017

src/main/java/io/appium/java_client/pagefactory/bys/builder/ByAll.java

+
+    @Override
+    public WebElement findElement(SearchContext context) {
+        for (By by : bys) {


return bys.stream() .map(x -> getSearchingFunction(x).apply(context)) .filter(Optional::isPresent) .findFirst() .orElseThrow(() -> new NoSuchElementException("Cannot locate an element using " + toString()));

@mykola-mokhnach I don't think that we can use stream in this case, because it returns By type not WebElement.

If you know how to need to do it without double invoking searching function, show me please.

we can. Updated my comment above

mykola-mokhnach · Jul 28, 2017

src/main/java/io/appium/java_client/pagefactory/bys/builder/ByAll.java

+
+public class ByAll extends org.openqa.selenium.support.pagefactory.ByAll {
+
+    private By[] bys;


I'd keep it as List<By> internally

mykola-mokhnach · Jul 28, 2017

src/main/java/io/appium/java_client/pagefactory/bys/builder/ByAll.java

+
+public class ByAll extends org.openqa.selenium.support.pagefactory.ByAll {
+
+    private List<By> bys;


can be final

mykola-mokhnach · Jul 28, 2017

src/main/java/io/appium/java_client/pagefactory/bys/builder/ByAll.java

+    private Function<SearchContext, Optional<WebElement>> getSearchingFunction(By by) {
+        return input -> {
+            try {
+                return input.findElement(by);


return Optional.of(input.findElement(by))

mykola-mokhnach · Jul 28, 2017

src/main/java/io/appium/java_client/pagefactory/bys/builder/ByAll.java

+                .filter(Optional::isPresent)
+                .findFirst()
+                .orElseThrow(() -> new NoSuchElementException("Cannot locate an element using " + toString()))
+                .orElse(null);


why this is needed?

This is double Optional : Optional<Optional<WebElement>. First orElseThrow for filter, the second orElse for searching function. I can use get instead orElse, because we already check Optional::isPresent, but there is warning here: "Optional.get()' without 'isPresent()' check"

This would be easier to read then

bys.stream() .map(by -> getSearchingFunction(by).apply(context)) .filter(Optional::isPresent) .map(Optional::get) .findFirst() .orElseThrow(() -> new NoSuchElementException("Cannot locate an element using " + toString()));

I need a vacation

functional programming makes fun :)

mykola-mokhnach · Jul 28, 2017

src/main/java/io/appium/java_client/pagefactory/bys/builder/ByAll.java

+        checkNotNull(bys);
+
+        this.bys = Arrays.asList(bys);
+        if (this.bys.isEmpty()) {


you can also use http://google.github.io/guava/releases/snapshot/api/docs/com/google/common/base/Preconditions.html#checkArgument(boolean) since checkNotNull is already there

mykola-mokhnach · Jul 28, 2017

BTW, do we have any functional or unit test, which covers this method?

mykola-mokhnach · Jul 28, 2017

src/main/java/io/appium/java_client/pagefactory/bys/builder/ByAll.java

+        return bys.stream()
+                .map(by -> getSearchingFunction(by).apply(context))
+                .filter(Optional::isPresent)
+                .findFirst().map(Optional::get)


mapping should be done before findFirst

TikhomirovSergey · Jul 28, 2017

@HlebHalkouski Hi. I like this idea.
@mykola-mokhnach Yes. We have such tests.
https://github.com/appium/java-client/blob/master/src/test/java/io/appium/java_client/pagefactory_tests/AndroidPageObjectTest.java#L348
https://github.com/appium/java-client/blob/master/src/test/java/io/appium/java_client/pagefactory_tests/SelendroidModeTest.java#L167
https://github.com/appium/java-client/blob/master/src/test/java/io/appium/java_client/pagefactory_tests/IOSPageFactoryTest.java#L233

TikhomirovSergey · Jul 29, 2017

@mykola-mokhnach So can we merge it or do you have some concerns/remarks?

mykola-mokhnach · Jul 29, 2017

@TikhomirovSergey There was only one minor comment about checkArgument usage. Everything else looks good.

TikhomirovSergey · Aug 2, 2017

@HlebHalkouski @mykola-mokhnach I will merge it soon.

HlebHalkouski · Aug 2, 2017

@TikhomirovSergey ok, Thank you!

By All was re-implemented, now it returns the first founded element f…

d1a9e51

…or single search.

mykola-mokhnach reviewed Jul 28, 2017

View reviewed changes

Fixes after review

a06a929

mykola-mokhnach reviewed Jul 28, 2017

View reviewed changes

Fixes after review

5b7955b

mykola-mokhnach reviewed Jul 28, 2017

View reviewed changes

Fixes after review

0cf1f9e

mykola-mokhnach reviewed Jul 28, 2017

View reviewed changes

Fixes after review

cef9efe

TikhomirovSergey approved these changes Jul 28, 2017

View reviewed changes

SrinivasanTarget approved these changes Jul 29, 2017

View reviewed changes

Finish fixes after review

3917b52

mykola-mokhnach approved these changes Aug 2, 2017

View reviewed changes

TikhomirovSergey mentioned this pull request Aug 3, 2017

PR issue fix. #686

Merged

TikhomirovSergey merged commit 3917b52 into appium:master Aug 3, 2017


		private By[] bys;

		private Function<SearchContext, WebElement> getSearchingFunction(By by) {


		public class ByAll extends org.openqa.selenium.support.pagefactory.ByAll {

		private By[] bys;


		public class ByAll extends org.openqa.selenium.support.pagefactory.ByAll {

		private List<By> bys;

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

Uh oh!

Conversation

HlebHalkouski commented Jul 28, 2017

Change list

Types of changes

Details

Uh oh!

jsf-clabot commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mykola-mokhnach Jul 28, 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!

mykola-mokhnach Jul 28, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey commented Jul 28, 2017

Uh oh!

TikhomirovSergey commented Jul 29, 2017

Uh oh!

mykola-mokhnach commented Jul 29, 2017

Uh oh!

TikhomirovSergey commented Aug 2, 2017

Uh oh!

HlebHalkouski commented Aug 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jsf-clabot commented Jul 28, 2017 •

edited

Loading

mykola-mokhnach Jul 28, 2017 •

edited

Loading

mykola-mokhnach Jul 28, 2017 •

edited

Loading