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

HlebHalkouski · 2017-07-28T09:13:35Z

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 · 2017-07-28T09:13:36Z

All committers have signed the CLA.

mykola-mokhnach · 2017-07-28T09:17:27Z

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


better to use isEmpty()

mykola-mokhnach · 2017-07-28T09:21:03Z

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


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

mykola-mokhnach · 2017-07-28T09:21:21Z

+
+ 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 · 2017-07-28T09:28:10Z

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


returnbys.stream() .map(x -> getSearchingFunction(x).apply(context)) .filter(Optional::isPresent) .findFirst() .orElseThrow(() -> newNoSuchElementException("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 · 2017-07-28T09:31:43Z

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


I'd keep it as List<By> internally

mykola-mokhnach · 2017-07-28T10:05:11Z

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


can be final

mykola-mokhnach · 2017-07-28T10:05:41Z

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


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

mykola-mokhnach · 2017-07-28T11:01:01Z

+ .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(() -> newNoSuchElementException("Cannot locate an element using " + toString()));

I need a vacation

functional programming makes fun :)

mykola-mokhnach · 2017-07-28T11:06:38Z

+ 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 · 2017-07-28T12:22:59Z

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

mykola-mokhnach · 2017-07-28T13:52:55Z

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


mapping should be done before findFirst

TikhomirovSergey · 2017-07-28T21:51:40Z

@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 · 2017-07-29T20:24:20Z

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

mykola-mokhnach · 2017-07-29T20:56:46Z

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

TikhomirovSergey · 2017-08-02T05:39:34Z

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

HlebHalkouski · 2017-08-02T07:00:06Z

@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:masterAug 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;

Uh oh!

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-mokhnachJul 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-mokhnachJul 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-mokhnachJul 28, 2017•
edited
Loading

mykola-mokhnachJul 28, 2017•
edited
Loading