Skip to content
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

#565 FIX #646

Merged
merged 9 commits into from
May 30, 2017
Merged

#565 FIX #646

merged 9 commits into from
May 30, 2017

Conversation

TikhomirovSergey
Copy link
Contributor

Change list

#565 FIX. The new feature implementation.

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)

Details

There annotations were re-designed (some annotations were added)

  • AndroidFindAll
  • AndroidFindBys
  • iOSFindAll
  • iOSFindBys
  • iOSXCUITFindAll
  • iOSXCUITFindBys
  • WindowsFindAll
  • WindowsFindBys

All these annotations are repeateble. The use case is described here.

@TikhomirovSergey
Copy link
Contributor Author

@SrinivasanTarget @amedvedjev could you take a look at the PR?

@amedvedjev
Copy link

@HowToUseLocators(androidAutomation = CHAIN, iOSAutomation = ALL_POSSIBLE)
@FINDALL{@findby(someStrategy1), @findby(someStrategy2)})

last ")" ?

same:
@HowToUseLocators(iOSAutomation = ALL_POSSIBLE)
@FINDALL{@findby(someStrategy1), @findby(someStrategy2)})

otherwise nice

@TikhomirovSergey
Copy link
Contributor Author

@amedvedjev Are you talking about the WIKI page?

@TikhomirovSergey TikhomirovSergey added this to the 5.0.0 milestone May 22, 2017
@amedvedjev
Copy link

@TikhomirovSergey yes. on wiki.

@TikhomirovSergey
Copy link
Contributor Author

@amedvedjev @SrinivasanTarget
This PR was updated. Could you take a look at this one more time?

@SrinivasanTarget could you please approve it if everything is ok?

Copy link
Member

@SrinivasanTarget SrinivasanTarget left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few Comments in readme. Overall cool stuff. 👍

@HowToUseLocators(iOSAutomation = ALL_POSSIBLE)
@iOSFindBy(someStrategy1)
@iOSFindAll(value = {@iOSBy(subloctor1), @iOSBy(subloctor1)}, priority = 1) //this possible variant is
// the chain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update this section as below please?

@HowToUseLocators(iOSAutomation = ALL_POSSIBLE)
 @iOSFindBy(someStrategy1)
 @iOSFindBys({
         @iOSBy(parentLocator),
         @iOSBy(childLocator)},
         priority = 1) //this possible variant is the chain
 @iOSFindBy(someStrategy2, priority = 2) 
 @iOSFindBy(someStrategy3, priority = 3) 
 RemoteWebElement someElement;

ALL_POSSIBLE contains CHAIN

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@iOSFindBy(someStrategy2, priority = 2)
@iOSFindBy(someStrategy3, priority = 3)
RemoteWebElement someElement;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHAINING Contains ALL_POSSIBLE. Awesome👍

@HowToUseLocators(iOSAutomation = ALL_POSSIBLE)
@iOSFindBy(someStrategy1)
@iOSFindAll(value = {@iOSBy(subloctor1), @iOSBy(subloctor1)}, priority = 1) //this possible variant is
// the chain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here too.

if (selendroidFindByArray != null && selendroidFindByArray.length == 1) {
return createBy(new Annotation[] {selendroidFindByArray[0]}, HowToUseSelectors.USE_ONE);
}
List<Annotation> annotations = new ArrayList<>(asList(annotatedElement.getAnnotationsByType(singleLocator)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra space here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I am working on the improving.

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) The code is very well written. Although things like this bother my internal perfectionist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mykola-mokhnach I have noticed that space. Ok. Thak you.

if (selendroidFindBys != null) {
return createBy(selendroidFindBys.value(), HowToUseSelectors.BUILD_CHAINED);
}
Annotation[] annotationsArray = annotations.toArray(new Annotation[]{});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any particular reason to transform it back to array? lists can also be sorted without any problems

Copy link
Contributor Author

@TikhomirovSergey TikhomirovSergey May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mykola-mokhnach Good catch. This is something like "legacy". I supposed to use array at first revisions.

}
Annotation[] annotationsArray = annotations.toArray(new Annotation[]{});
sort(annotationsArray, comparator);
By[] result = new By[] {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here - why not to use list and only convert it to array on return?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mykola-mokhnach This is not the same. Array is returned by java by design. Comporator can work with collection and array so why we have to covert it to a list?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result = add(result, createBy(new Annotation[] {a}, HowToUseSelectors.USE_ONE));

I suppose this will create a new copy of the array on each call and this is not the case for a list where we can add elements to the same instance.

if (androidFindByArray != null && androidFindByArray.length == 1) {
return createBy(new Annotation[] {androidFindByArray[0]}, HowToUseSelectors.USE_ONE);
Method value;
Annotation[] set;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this variable have some more descriptive name?

}
By result = null;
if (isSelendroidAutomation()) {
result = buildMobileBy(howToUseLocators != null ? howToUseLocators.selendroidAutomation() : null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always a bit suspicious about using method calls with ternary operator, since Java will invoke the method even if the precondition equals to false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question to all the further ternary operator usages in such context (with method invocation instead of constant values)

if (iOSFindBys != null) {
return createBy(iOSFindBys.value(), HowToUseSelectors.BUILD_CHAINED);
}
if (isAndroid() && result == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exchanging operands

result == null && iisAndroid()

could make it faster

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general one can simplify it by adding

if (result != null) {
   return result;
}

before all these conditions. Then it won't be necessary to check whether it is equal to null in all the further conditions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mykola-mokhnach There are some difficulties.
It is by design. When selendroind-mode-specific annotations are not defined there when it is possible to use '@AndroidFindBy-s' at some cases. The same is true for iOSXCUITFindBy-s/iOSFindBy-s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...however I know how to make to look it better.

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to say that isAndroid() method (and others) may implicitly change the value of result?

int p1 = getPriorityValue(priority1, o1, c1);
int p2 = getPriorityValue(priority2, o2, c2);

if (p2 > p1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole condition can be replaced with

return Integer.signum(p1 - p2);

@TikhomirovSergey
Copy link
Contributor Author

TikhomirovSergey commented May 29, 2017

@mykola-mokhnach @SrinivasanTarget
Required changes have been done. Could you rewiew the PR one more time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants