Improve the error message on session creation (#994)

* Improve the error message on session creation

* Use single quotes

* Better error message for InvocationTargetException exception

* fix checkstyle

Author: Mykola Mokhnach

build.gradle

@@ -7,6 +7,8 @@ apply plugin: 'checkstyle'
 apply plugin: 'signing'
 apply plugin: 'maven-publish'
 
+import org.apache.tools.ant.filters.*
+
 group 'io.appium'
 version '6.1.0'
 
@@ -50,21 +52,19 @@ compileJava {
     ]
 }
 
-ext.seleniumVersion = '3.14.0'
-
 dependencies {
-    compile ("org.seleniumhq.selenium:selenium-java:${seleniumVersion}") {
+    compile ("org.seleniumhq.selenium:selenium-java:${project.property('selenium.version')}") {
         force = true
 
         exclude group: 'com.google.code.gson'
         exclude module: 'htmlunit-driver'
         exclude group: 'net.sourceforge.htmlunit'
 
     }
-    compile ("org.seleniumhq.selenium:selenium-support:${seleniumVersion}") {
+    compile ("org.seleniumhq.selenium:selenium-support:${project.property('selenium.version')}") {
         force = true
     }
-    compile ("org.seleniumhq.selenium:selenium-api:${seleniumVersion}") {
+    compile ("org.seleniumhq.selenium:selenium-api:${project.property('selenium.version')}") {
         force = true
     }
     compile 'com.google.code.gson:gson:2.8.4'
@@ -199,6 +199,12 @@ wrapper {
     distributionType = Wrapper.DistributionType.ALL
 }
 
+processResources {
+    filter ReplaceTokens, tokens: [
+            'selenium.version': project.property('selenium.version')
+    ]
+}
+
 task xcuiTest( type: Test ) {
     useJUnit()
     testLogging.showStandardStreams = true

gradle.properties

@@ -6,3 +6,5 @@ signing.secretKeyRingFile=PathToYourKeyRingFile
 
 ossrhUsername=your-jira-id
 ossrhPassword=your-jira-password
+
+selenium.version=3.14.0

src/main/java/io/appium/java_client/internal/Config.java

@@ -0,0 +1,71 @@
+package io.appium.java_client.internal;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.UncheckedIOException;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class Config {
+    private static Config mainInstance = null;
+    private static final String MAIN_CONFIG = "main.properties";
+    private static final Map<String, Properties> cache = new ConcurrentHashMap<>();
+    private final String configName;
+
+    /**
+     * Retrieve a facade for the main config.
+     *
+     * @return interaction helper for 'main.properties' config
+     */
+    public static synchronized Config main() {
+        if (mainInstance == null) {
+            mainInstance = new Config(MAIN_CONFIG);
+        }
+        return mainInstance;
+    }
+
+    private Config(String configName) {
+        this.configName = configName;
+    }
+
+    /**
+     * Retrieve a value from properties file.
+     *
+     * @param key the name of the corresponding key which value to retrieve
+     * @param valueType the expected type of the value to be retrieved
+     * @return the actual value
+     * @throws IllegalArgumentException if the given key does not exist
+     * @throws ClassCastException if the retrieved value cannot be cast to `valueType` type
+     */
+    public <T> T getValue(String key, Class<T> valueType) {
+        return getOptionalValue(key, valueType)
+                .orElseThrow(() -> new IllegalArgumentException(
+                        String.format("There is no '%s' key in '%s' config", key, configName)
+                ));
+    }
+
+    /**
+     * Retrieve a value from properties file.
+     *
+     * @param key the name of the corresponding key which value to retrieve
+     * @param valueType the expected type of the value to be retrieved
+     * @return the actual value or {@link Optional#empty()} if the key is not present
+     * @throws UncheckedIOException if the given properties file does not exist/not accessible
+     * @throws ClassCastException if the retrieved value cannot be cast to `valueType` type
+     */
+    public <T> Optional<T> getOptionalValue(String key, Class<T> valueType) {
+        final Properties cachedProps = cache.computeIfAbsent(configName, (k) -> {
+            try (InputStream configFileStream = getClass().getClassLoader().getResourceAsStream(configName)) {
+                final Properties p = new Properties();
+                p.load(configFileStream);
+                return p;
+            } catch (IOException e) {
+                throw new UncheckedIOException(String.format("Configuration file '%s' cannot be loaded",
+                        configName), e);
+            }
+        });
+        return cachedProps.containsKey(key) ? Optional.of(valueType.cast(cachedProps.get(key))) : Optional.empty();
+    }
+}

src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java

@@ -30,6 +30,7 @@
 import com.google.common.io.CountingOutputStream;
 import com.google.common.io.FileBackedOutputStream;
 
+import io.appium.java_client.internal.Config;
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.ImmutableCapabilities;
 import org.openqa.selenium.SessionNotCreatedException;
@@ -190,17 +191,21 @@ public Result createSession(HttpClient client, Command command)
                                     .info(format("Detected dialect: %s", toReturn.getDialect()));
                             return toReturn;
                         }).orElseThrow(() -> new SessionNotCreatedException(
-                                format("Unable to create new remote session. desired capabilities = %s", desired)));
+                                format("Unable to create a new remote session. Desired capabilities = %s", desired)));
                     } catch (NoSuchMethodException | IllegalAccessException e) {
-                        throw new WebDriverException(format("It is impossible to create a new session "
-                                        + "because 'createSession' which takes %s, %s and %s was not found "
-                                        + "or it is not accessible",
-                                HttpClient.class.getSimpleName(),
-                                InputStream.class.getSimpleName(),
-                                long.class.getSimpleName()), e);
+                        throw new SessionNotCreatedException(format("Unable to create a new remote session. "
+                                        + "Make sure your project dependencies config does not override "
+                                        + "Selenium API version %s used by java-client library.",
+                                Config.main().getValue("selenium.version", String.class)), e);
                     } catch (InvocationTargetException e) {
-                        throw new SessionNotCreatedException(
-                                format("Unable to create new remote session. Desired capabilities: %s", desired), e);
+                        String message = "Unable to create a new remote session.";
+                        if (e.getCause() != null) {
+                            if (e.getCause() instanceof WebDriverException) {
+                                message += " Please check the server log for more details.";
+                            }
+                            message += format(" Original error: %s", e.getCause().getMessage());
+                        }
+                        throw new SessionNotCreatedException(message, e);
                     }
                 } finally {
                     os.reset();

src/main/resources/main.properties

@@ -0,0 +1 @@
+selenium.version[email protected]@

src/test/java/io/appium/java_client/internal/ConfigTest.java

@@ -0,0 +1,34 @@
+package io.appium.java_client.internal;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.greaterThan;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+public class ConfigTest {
+    private static final String EXISTING_KEY = "selenium.version";
+    private static final String MISSING_KEY = "bla";
+
+    @Test
+    public void verifyGettingExistingValue() {
+        assertThat(Config.main().getValue(EXISTING_KEY, String.class).length(), greaterThan(0));
+        assertTrue(Config.main().getOptionalValue(EXISTING_KEY, String.class).isPresent());
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void verifyGettingNonExistingValue() {
+        assertThat(Config.main().getValue(MISSING_KEY, String.class).length(), greaterThan(0));
+    }
+
+    @Test(expected = ClassCastException.class)
+    public void verifyGettingExistingValueWithWrongClass() {
+        assertThat(Config.main().getValue(EXISTING_KEY, Integer.class), greaterThan(0));
+    }
+
+    @Test
+    public void verifyGettingNonExistingOptionalValue() {
+        assertFalse(Config.main().getOptionalValue(MISSING_KEY, String.class).isPresent());
+    }
+}