fix: #1660 - correction for ping method to get proper status URL in case of a basepath provided by robinGupta11392 · Pull Requ 10BC0 est #1661 · appium/java-client

robinGupta11392 · 2022-03-17T10:15:52Z

Change list

Fixed the logic of appending basepath to the Hub URL which fixes Issue #1660 - #1660.

AppiumDriverLocalService was not able to start with Appium CLI versions < 2 as the ping() method was not able to get the status URL properly. Fixed the logic of appending basepath to sanitise the user input and appending basepath with "/" in the end, if not provided by user.

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

Test Execution logs - https://gist.github.com/robinGupta11392/e8c37c7e10e03cd00a4d44d398402738
Issue fixed - #1660
Changed Classes:

ServerBuilderTest
AppiumDriverLocalService

Method Added:

sanitizeBasePath

Method Modified:

withBasePath

Test Added:

checkAbilityToStartServiceUsingBasePath

…ervice is not able to start - checkStatus is failing

Fix for basePath appending logic to fix Issue appium#1660

linux-foundation-easycla · 2022-03-17T10:15:55Z

✅ login: robinGupta11392 (c19a926, c553b91, 69d2a41, e013f70, 3061c99, 5b76ab9)
❌ The commit (e8da284, 72cf29f). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

src/main/java/io/appium/java_client/service/local/AppiumDriverLocalService.java

…LocalService.java Removing redundant parenthesis Co-authored-by: Valery Yatsynovich <valfirst@yandex.ru>

src/main/java/io/appium/java_client/service/local/AppiumDriverLocalService.java

…reviews - replaced String#isBlank with StringUtils#isBlank for Java language compatibility - introduced InvalidBasePathException to be thrown for blank/null/empty values of basepath - basePath validations moved to the step of storing server arguments to give forward compatibility with Appium v2 fix: added extra test cases for more precisely testing basepath validations

robinGupta11392 · 2022-03-18T07:57:38Z

https://gist.github.com/robinGupta11392/19cb22749170119d791cbeb8e1a714b3 --> ServerBuilderTest execution logs with Appium V2 Beta 25 post the fix. It seems there is one Log test failing right from the moment i cloned it - maybe you guys can have a look for reference.

src/main/java/io/appium/java_client/service/local/AppiumServiceBuilder.java

src/test/java/io/appium/java_client/service/local/ServerBuilderTest.java

src/main/java/io/appium/java_client/service/local/AppiumServiceBuilder.java

src/test/java/io/appium/java_client/service/local/ServerBuilderTest.java

removed redundant validation of StringUtils#isBlank() on the basepath replaced Assert#fail with Assert#assertThrows for the test cases

src/test/java/io/appium/java_client/service/local/ServerBuilderTest.java

src/main/java/io/appium/java_client/service/local/AppiumServiceBuilder.java

src/test/java/io/appium/java_client/service/local/ServerBuilderTest.java

…eBuilder.java Co-authored-by: Valery Yatsynovich <valfirst@yandex.ru>

Removed validations of exception message in the tests Accepted the change from val to use checkArgument method from Preconditions class

mykola-mokhnach · 2022-03-19T07:23:04Z

src/test/java/io/appium/java_client/service/local/ServerBuilderTest.java

+
+    @Test
+    public void checkAbilityToValidateBasePathForEmptyBasePath() {
+        assertThrows(IllegalArgumentException.class, () -> {


FYI: you could add expected exception signature to Test annotations: https://www.guru99.com/junit-exception-test.html

Noted with thanks.

“robin.gupta” and others added 3 commits March 17, 2022 16:04

Fix for basePath appending logic to fix Issue#1660 AppiumDriverLocalS…

e8da284

…ervice is not able to start - checkStatus is failing

Added test to check the service instantiation with basepath

72cf29f

Merge pull request #1 from robinGupta11392/1660-fix-for-checkStatus-url

c19a926

Fix for basePath appending logic to fix Issue appium#1660

robinGupta11392 mentioned this pull request Mar 17, 2022

AppiumDriverLocalService is not able to start - checkStatus is failing #1660

Closed

robinGupta11392 changed the title ~~Fix for Issue #1660 - correction for ping method to get proper status URL in case of a basepath provided~~ Fix: #1660 - correction for ping method to get proper status URL in case of a basepath provided Mar 17, 2022

valfirst requested changes Mar 17, 2022

View reviewed changes

src/main/java/io/appium/java_client/service/local/AppiumDriverLocalService.java Outdated Show resolved Hide resolved

src/main/java/io/appium/java_client/service/local/AppiumDriverLocalService.java Outdated Show resolved Hide resolved

mykola-mokhnach reviewed Mar 17, 2022

View reviewed changes

src/main/java/io/appium/java_client/service/local/AppiumDriverLocalService.java Outdated Show resolved Hide resolved

mykola-mokhnach reviewed Mar 17, 2022

View reviewed changes

src/main/java/io/appium/java_client/service/local/AppiumDriverLocalService.java Outdated Show resolved Hide resolved

Update src/main/java/io/appium/java_client/service/local/AppiumDriver…

c553b91

…LocalService.java Removing redundant parenthesis Co-authored-by: Valery Yatsynovich <valfirst@yandex.ru>

mykola-mokhnach changed the title ~~Fix: #1660 - correction for ping method to get proper status URL in case of a basepath provided~~ fix: #1660 - correction for ping method to get proper status URL in case of a basepath provided Mar 18, 2022