The addition to #738 #741
The addition to #738 #741
Conversation
- following dependencies were updated: `org.seleniumhq.selenium:selenium-java` to 3.6.0 `com.google.code.gson:gson` to 2.8.2 `org.springframework:spring-context` to 5.0.0.RELEASE `org.aspectj:aspectjweaver` to 1.8.11 - unused parameters and fields were removed from AppiumElementLocator, AppiumElementLocatorFactory - some test improving
TikhomirovSergey
requested review from
SrinivasanTarget,
mykola-mokhnach and
titusfortner
titusfortner
left a comment
titusfortner
left a comment
There was a problem hiding this comment.
This looks like it incorporates the stuff I care about. I just have that one comment that doesn't affect the functionality. :)
|
|
||
| @AndroidFindBy(className = "someClass") @AndroidFindBy(xpath = "//someTag") | ||
| private RemoteWebElement btnG; //this element should be found by id = 'btnG' or name = 'btnG' | ||
| private RemoteWebElement btnK; //this element should be found by id = 'btnG' or name = 'btnG' |
There was a problem hiding this comment.
If this is changed, then the comment should also be changed?
There was a problem hiding this comment.
ok. I will improve it a bit later.
|
|
||
| public AppiumElementLocator(SearchContext searchContext, By by, boolean shouldCache, | ||
| TimeOutDuration duration, TimeOutDuration originalDuration, WebDriver originalWebDriver) { | ||
| TimeOutDuration duration, TimeOutDuration originalDuration) { |
There was a problem hiding this comment.
would it be possible to also replace the custom TimeOutDuration class with the native Duration one, since we anyway change method signature?
There was a problem hiding this comment.
@mykola-mokhnach
I think these parameters are needed still.
TimeOutDuration is just the container of duration values.
https://github.com/appium/java-client/blob/master/src/main/java/io/appium/java_client/pagefactory/TimeOutDuration.java
It was designed for the purposes like
https://github.com/TikhomirovSergey/java-client/blob/73077776d513e4521d7d5e8304469a4b85232e05/src/test/java/io/appium/java_client/pagefactory_tests/TimeOutTest.java#L122
I think it is more useful to completely change value than do 'plus' or 'minus'.
However, I think I could redesign TimeOutDuration and make it work with java.time.Duration. I think it should be done step by step. The first step is to mark some things @Deprecated and to add new good things instead, the second step is removal.
I just think that the change of TimeOutDuration may impact many users.
There was a problem hiding this comment.
yep, I agree about we can do a separate PR for it
| * @param builder is handler of Appium-specific page object annotations | ||
| */ | ||
|
|
||
| public AppiumElementLocatorFactory(SearchContext searchContext, TimeOutDuration timeOutDuration, |
There was a problem hiding this comment.
same comment as above
| RemoteWebElement.class, MobileElement.class, AndroidElement.class, | ||
| IOSElement.class, WindowsElement.class); | ||
| public static long DEFAULT_IMPLICITLY_WAIT_TIMEOUT = 1; | ||
| public static long DEFAULT_TIMEOUT = 1; |
There was a problem hiding this comment.
we can deprecate the constant of TimeUnit type if we use Duration type for DEFAULT_TIMEOUT
|
|
||
|
|
||
| public AppiumFieldDecorator(SearchContext context, long implicitlyWaitTimeOut, | ||
| public AppiumFieldDecorator(SearchContext context, long timeOut, |
| import java.nio.file.Path; | ||
|
|
||
| public final class ChromeDriverPathUtil { | ||
| private static final Path ROOT_TEST_PATH = getDefault().getPath("src") |
| Path resultPath; | ||
| Platform current = getCurrent(); | ||
|
|
||
| if (current.is(WINDOWS)) { |
There was a problem hiding this comment.
why cannot we use switch () case ...: return here? this will allow to get rid of many temporary variables
There was a problem hiding this comment.
Because there may be WINDOWS8, WINDOWS10, WINDOWS_XP etc. I think that it is easier to compare current platforn with WINDOWS (the result for WINDOWS8, WINDOWS10, WINDOWS_XP is TRUE)
There was a problem hiding this comment.
np if you prefer this. However it still can be simplified by retuning the value immediately inside if block
| import java.util.List; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| public class TimeOutTest { |
|
|
||
| public class TimeOutTest { | ||
|
|
||
| private static final long ACCEPTABLE_DELTA_MILLS = 1500; |
|
|
||
| private TimeOutDuration timeOutDuration; | ||
|
|
||
| private static long getBenchMark(List<WebElement> stubElements) { |
| private TimeOutDuration timeOutDuration; | ||
|
|
||
| private static long getBenchMark(List<WebElement> stubElements) { | ||
| long startMark = Calendar.getInstance().getTimeInMillis(); |
There was a problem hiding this comment.
I usually use System.nanoTime() for such purpose or System.currentTimeMillis() if we don't need nano-preciseness. Calendar class has different purpose.
There was a problem hiding this comment.
yes :) it was taken from the old code of the test. will improve it.
|
|
||
| private TimeOutDuration timeOutDuration; | ||
|
|
||
| private static long getBenchMark(List<WebElement> stubElements) { |
There was a problem hiding this comment.
One can make this method more generic by providing a lambda function to benchmark:
private static Duration getBenchmark(Runnable dstFunc) {
final long startTime = System.currentTimeMillis();
dstFunc.run();
return Duration.ofMillis(System.currentTimeMillis() - startTime);
}
|
@mykola-mokhnach This PR was updated. |
|
|
||
| return ofNullable(byResult) | ||
| .map(by -> new AppiumElementLocator(searchContext, by, builder.isLookupCached(), customDuration)) | ||
| .orElse(null); |
There was a problem hiding this comment.
The method can be marked as Nullable
| return ROOT_TEST_PATH.resolve("chromedriver.exe").toFile(); | ||
| } else if (current.is(MAC)) { | ||
| return ROOT_TEST_PATH.resolve("chromedriver_mac").toFile(); | ||
| } else { |
There was a problem hiding this comment.
this else is not needed
|
|
||
| @Test public void defaultTimeOutTest() { | ||
| assertThat(format(MESSAGE, DEFAULT_TIMEOUT, DEFAULT_TIMEUNIT), | ||
| abs(getExpectedMillis(DEFAULT_TIMEOUT, DEFAULT_TIMEUNIT) - getBenchmark(() -> stubElements.size())), |
There was a problem hiding this comment.
I assume the whole formula can be extracted into a separate method, for example:
private static long getPerformanceDiff(long expectedMs, Runnable m) {
return abs(expectedMs - getBenchmark(m));
}|
|
||
| public class TimeoutTest { | ||
|
|
||
| private static final long ACCEPTABLE_TIME_DIFF = 1500; |
There was a problem hiding this comment.
It is a good practice to explicitly mention measurement unit in var name in case it is of primitive data type (e.g. _MS, _SEC)
mykola-mokhnach
left a comment
mykola-mokhnach
left a comment
There was a problem hiding this comment.
minor comments only
|
Replacing DIFF with DELTA might be easier to read. Otherwise LGTM |
3 participants
Change list
The addition to #738
Types of changes
Details
The addition to #738 …
following dependencies were updated:
org.seleniumhq.selenium:selenium-javato 3.6.0com.google.code.gson:gsonto 2.8.2org.springframework:spring-contextto 5.0.0.RELEASEorg.aspectj:aspectjweaverto 1.8.11unused parameters and fields were removed from AppiumElementLocator,
AppiumElementLocatorFactory
some test improving