Tests of local appium DriverService were re-designed. #728

TikhomirovSergey · 2017-09-16T22:08:42Z

Change list

Test of the appium-specific DriverService implementation were improved/redesigned.

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

We are going to improve tests and make them runnable on many environments.
#325 #494

mykola-mokhnach · 2017-09-17T06:19:18Z

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

+
+public class ServerBuilderTest {
+
+    private static String pathToAppiumNodeInProperties;


probably, we can mark properties, which are not going to be changed, as final

mykola-mokhnach · 2017-09-17T06:21:32Z

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

+                    testIP = null;
+                }
+            }
+            if (testIP != null) {


I assume having this in a separate private method would save us several lines of code (break -> return) and will improve readability.

the returned result can be of type Optional

mykola-mokhnach · 2017-09-17T06:22:47Z

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

+        }
+
+        ofNullable(file).ifPresent(fileArg -> {
+            if (file.exists()) {


file or fileArg?

mykola-mokhnach · 2017-09-17T06:31:25Z

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

+    }
+
+    @Test public void checkAbilityToUseNodeDefinedExplicitly() {
+        File node = new File("src/test/java/io/appium/java_client/service/local/appium.js");


the magic string can be moved to constants

mykola-mokhnach · 2017-09-17T06:33:15Z

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

+    }
+
+    @Test public void checkAbilityToStartServiceUsingCapabilities() throws Exception {
+        File appDir = new File("src/test/java/io/appium/java_client");


there are many common root paths. The common part can be represented as a constant, so then one can use new File(parent, basename) constructor, where parent is a constant

mykola-mokhnach · 2017-09-17T06:35:05Z

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

+        service = buildDefaultService();
+        service.addOutPutStream(stream);
+        service.start();
+        assertTrue(file.length() > 0);


-> assertThat(file.length(), greaterThan(0L))

this is a bit longer, but produces better error message in comparison to the generic True assertion

mykola-mokhnach · 2017-09-17T06:36:49Z

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

+                new AppiumServiceBuilder().usingAnyFreePort().build(),
+                new AppiumServiceBuilder().usingAnyFreePort().build(),
+                new AppiumServiceBuilder().usingAnyFreePort().build());
+        services.parallelStream().forEach(AppiumDriverLocalService::start);


mykola-mokhnach · 2017-09-17T06:37:49Z

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

+        service = new AppiumServiceBuilder().withLogFile(file).build();
+        service.start();
+        assertTrue(file.exists());
+        assertTrue(file.length() > 0);


same as above

mykola-mokhnach · 2017-09-17T06:41:42Z

Shall we add a description somewhere about src/test/java/io/appium/java_client/service/local/appium.js is just a stub?

mykola-mokhnach

see above

TikhomirovSergey · 2017-09-17T20:00:32Z

@mykola-mokhnach @SrinivasanTarget
Code was improved

SrinivasanTarget · 2017-09-18T04:07:32Z

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

+                        if (validator.isValid(result)) {
+                            return result;
+                        }
+                        return null;


return validator.isValid(result) ? result : null;

mykola-mokhnach · 2017-09-18T05:39:19Z

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

+                if (!inetAddress.isLoopbackAddress()) {
+                    InetAddressValidator validator = InetAddressValidator.getInstance();
+
+                    testIP = ofNullable(testIP).orElseGet(() -> {


this won't work as expected, because the iteration does not stop here even if testIP set. Please extract the loop into a separate method private static Optional<String> getTestIP(), so then beforeClass's body should look like testIP = getTestIP().orElseThrow(() -> new IllegalStateException("Cannot initialize the IP address for tests"))

mykola-mokhnach · 2017-09-18T05:44:56Z

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

+     */
+    private static final String PATH_TO_APPIUM_NODE_IN_PROPERTIES = getProperty(APPIUM_PATH);
+
+    private static final String ROOT_TEST_PATH = "src/test/java/io/appium/java_client/";


trailing slash is not needed if path concatenation is done in File constructor

also, will these tests work properly in Windows if one hardcodes path separators?

mykola-mokhnach · 2017-09-18T05:50:18Z

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

+    /**
+     * This is the path to the stub appium.js file
+     */
+    private static final String PATH_T0_TEST_APPIUM_JS = ROOT_TEST_PATH + "service/local/appium.js";


It will be more handy to keep it as File object:
TEST_APPIUM_JS = Path.get(ROOT_TEST_PATH).resolve("service").resolve("local").resolve("appium.js").toFile()

@TikhomirovSergey Why don't we name stub as main.js in sync with appium server's main.js?

@SrinivasanTarget
Because it searches for appium.js when it tries to launch the node locally

@TikhomirovSergey with the latest appium its main.js as @SrinivasanTarget mentioned

https://github.com/appium/java-client/blob/master/src/main/java/io/appium/java_client/service/local/AppiumServiceBuilder.java#L70

@SrinivasanTarget @saikrishna321 Oops! My fault. yes. Guys you are right :)

mykola-mokhnach · 2017-09-18T06:04:05Z

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

+        });
+
+        System.clearProperty(APPIUM_PATH);
+        ofNullable(PATH_TO_APPIUM_NODE_IN_PROPERTIES).ifPresent(s -> setProperty(APPIUM_PATH, s));


the constant cannot be null

@mykola-mokhnach It may be null when the property was not defined

mykola-mokhnach · 2017-09-18T06:05:00Z

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

+
+    @Test public void checkAbilityToStartServiceUsingCapabilitiesAndFlags() throws Exception {
+        File app = new File(ROOT_TEST_PATH, "ApiDemos-debug.apk");
+        File chrome = new File(new File(ROOT_TEST_PATH, "pagefactory_tests"), "chromedriver.exe");


Use Path.resolve method to construct longer paths

TikhomirovSergey · 2017-09-18T07:58:45Z

Ok. Will improve it ASAP

TikhomirovSergey · 2017-09-19T20:34:55Z

@mykola-mokhnach @SrinivasanTarget
Could you take a look one more time. It seems it should be finally resolved.

mykola-mokhnach · 2017-09-20T05:35:43Z

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

+    private File testLogFile;
+    private OutputStream stream;
+
+    private static String getLocalIP(NetworkInterface intf) {


please add Nullable annotation

mykola-mokhnach · 2017-09-20T05:36:33Z

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

+                InetAddressValidator validator = InetAddressValidator.getInstance();
+                calculated = inetAddress.getHostAddress().toString();
+                if (validator.isValid(calculated)) {
+                    result = calculated;


return calculated

Will try it but codecy is sensetive to things like that

mykola-mokhnach · 2017-09-20T05:36:59Z

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

+                }
+            }
+        }
+        return result;


return null, result variable is not needed.

mykola-mokhnach · 2017-09-20T05:38:06Z

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

+        String result = null;
+        for (Enumeration<InetAddress> enumIpAddr = intf.getInetAddresses(); enumIpAddr
+                .hasMoreElements(); ) {
+            String calculated;


what's the point to define calculated before if condition?

mykola-mokhnach · 2017-09-20T05:39:25Z

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

+    public void tearDown() throws Exception {
+        ofNullable(service).ifPresent(AppiumDriverLocalService::stop);
+
+        if (stream != null) {


ofNullable(stream).ifPresent(OutputStream::close). Just to be consistent

It throws the checked IO exception that needs to be handled by catch. So I decided to change nothing there because lambda with the catch clause looks not so cool.

mykola-mokhnach · 2017-09-20T05:39:47Z

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

+    }
+
+    @Test public void checkAbilityToFindNodeDefinedInProperties() {
+        File definedNode = PATH_T0_TEST_MAIN_JS.toFile();


mykola-mokhnach

Minor comments. Otherwise all good

Tests of local appium DriverService were re-designed.

ea2c5c1

TikhomirovSergey requested review from SrinivasanTarget and mykola-mokhnach September 16, 2017 22:08

TikhomirovSergey self-assigned this Sep 16, 2017

TikhomirovSergey added the testing label Sep 16, 2017

mykola-mokhnach reviewed Sep 17, 2017

View reviewed changes

TikhomirovSergey added 3 commits September 17, 2017 22:41

ServerBuilderTest: code improvement.

61360cc

ServerBuilderTest: magic strings were turned into final values

c190315

ServerBuilderTest: magic strings were turned into final values

2b86b18

appium deleted a comment Sep 17, 2017

SrinivasanTarget reviewed Sep 18, 2017

View reviewed changes

mykola-mokhnach reviewed Sep 18, 2017

View reviewed changes

ServerBuilderTest: the path resolving

916afef

mykola-mokhnach reviewed Sep 20, 2017

View reviewed changes

mykola-mokhnach approved these changes Sep 20, 2017

View reviewed changes

SrinivasanTarget approved these changes Sep 20, 2017

View reviewed changes

ServerBuilderTest: ip calculation was improved

5e300d5

TikhomirovSergey merged commit 9e3946c into appium:master Sep 22, 2017


		public class ServerBuilderTest {

		private static String pathToAppiumNodeInProperties;

Uh oh!

Tests of local appium DriverService were re-designed. #728

Tests of local appium DriverService were re-designed. #728

Uh oh!

Conversation

TikhomirovSergey commented Sep 16, 2017

Change list

Types of changes

Details

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 Sep 17, 2017

Uh oh!

mykola-mokhnach left a comment

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey commented Sep 17, 2017

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!

saikrishna321 Sep 19, 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!

TikhomirovSergey Sep 19, 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!

TikhomirovSergey commented Sep 18, 2017

Uh oh!

TikhomirovSergey commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Sep 20, 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

saikrishna321 Sep 19, 2017 •

edited

Loading

TikhomirovSergey Sep 19, 2017 •

edited

Loading

mykola-mokhnach Sep 20, 2017 •

edited

Loading

TikhomirovSergey Sep 20, 2017 •

edited

Loading