Fixing processing of -hub, -hubHost and -hubPort options, -hub should have precedence. Fixes #5219

Author: barancev

java/server/src/org/openqa/grid/internal/utils/configuration/GridNodeConfiguration.java

@@ -132,6 +132,18 @@ static List<MutableCapabilities> getCapabilities() {
     }
   }
 
+  private static class HostPort {
+    final String host;
+    final int port;
+
+    HostPort(String host, int port) {
+      this.host = host;
+      this.port = port;
+    }
+  }
+
+  private HostPort hubHostPort;
+
   /*
    * config parameters which do not serialize or de-serialize
    */
@@ -228,11 +240,13 @@ static List<MutableCapabilities> getCapabilities() {
   /**
    * The hub url. Defaults to {@code http://localhost:4444}.
    */
-  @Expose
   @Parameter(
     names = "-hub",
     description = "<String> : the url that will be used to post the registration request. This option takes precedence over -hubHost and -hubPort options."
   )
+  private String hubOption;
+
+  @Expose
   public String hub = DEFAULT_HUB;
 
   /**
@@ -307,23 +321,42 @@ public GridNodeConfiguration() {
   }
 
   public String getHubHost() {
-    if (hubHost == null) {
-      if (hub == null) {
-        throw new RuntimeException("You must specify either a hubHost or hub parameter.");
-      }
-      parseHubUrl();
-    }
-    return hubHost;
+    return getHubHostPort().host;
   }
 
   public Integer getHubPort() {
-    if (hubPort == null) {
-      if (hub == null) {
-        throw new RuntimeException("You must specify either a hubPort or hub parameter.");
+    return getHubHostPort().port;
+  }
+
+  private HostPort getHubHostPort() {
+    if (hubHostPort == null) { // parse options
+      // -hub has precedence
+      if (hubOption != null) {
+        hub = hubOption;
+        try {
+          URL u = new URL(hub);
+          hubHostPort = new HostPort(u.getHost(), u.getPort());
+        } catch (MalformedURLException mURLe) {
+          throw new RuntimeException("-hub must be a valid url: " + hub, mURLe);
+        }
+      } else if (hubHost != null || hubPort != null) {
+        if (hubHost == null) {
+          throw new RuntimeException("You must specify either a -hubHost or -hub parameter.");
+        }
+        if (hubPort == null) {
+          throw new RuntimeException("You must specify either a -hubPort or -hub parameter.");
+        }
+        hubHostPort = new HostPort(hubHost, hubPort);
+      } else {
+        try {
+          URL u = new URL(hub);
+          hubHostPort = new HostPort(u.getHost(), u.getPort());
+        } catch (MalformedURLException mURLe) {
+          throw new RuntimeException("-hub must be a valid url: " + hub, mURLe);
+        }
       }
-      parseHubUrl();
     }
-    return hubPort;
+    return hubHostPort;
   }
 
   public String getRemoteHost() {
@@ -339,16 +372,6 @@ public String getRemoteHost() {
     return remoteHost;
   }
 
-  private void parseHubUrl() {
-    try {
-      URL u = new URL(hub);
-      hubHost = u.getHost();
-      hubPort = u.getPort();
-    } catch (MalformedURLException mURLe) {
-      throw new RuntimeException("-hub must be a valid url: " + hub, mURLe);
-    }
-  }
-
   public void merge(GridNodeConfiguration other) {
     if (other == null) {
       return;

java/server/test/org/openqa/grid/internal/utils/configuration/GridNodeConfigurationTest.java

@@ -22,6 +22,7 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
 import com.google.common.collect.ImmutableMap;
@@ -30,10 +31,12 @@
 
 import com.beust.jcommander.JCommander;
 
+import org.hamcrest.CoreMatchers;
 import org.junit.Test;
 import org.openqa.grid.common.exception.GridConfigurationException;
 import org.openqa.selenium.Platform;
 import org.openqa.selenium.remote.DesiredCapabilities;
+import org.openqa.selenium.testing.TestUtilities;
 
 import java.util.Arrays;
 
@@ -146,7 +149,6 @@ public void testConstructorEqualsDefaultConfig() {
     assertEquals(expected.nodeConfigFile, actual.nodeConfigFile);
     assertEquals(expected.unregisterIfStillDownAfter, actual.unregisterIfStillDownAfter);
 
-
     assertEquals(expected.cleanUpCycle, actual.cleanUpCycle);
     assertEquals(expected.host, actual.host);
     assertEquals(expected.maxSession, actual.maxSession);
@@ -217,16 +219,74 @@ public void testWithCapabilitiesArgsWithExtraSpacing() {
 
   @Test
   public void testGetHubHost() {
+    final String[] args = new String[]{"-hubHost", "dummyhost", "-hubPort", "1234"};
     GridNodeConfiguration gnc = new GridNodeConfiguration();
-    gnc.hub = "http://dummyhost:4444/wd/hub";
+    new JCommander(gnc, args);
     assertEquals("dummyhost", gnc.getHubHost());
   }
 
+  @Test
+  public void testGetHubHostFromHubOption() {
+    final String[] args = new String[]{"-hub", "http://dummyhost:1234/wd/hub"};
+    GridNodeConfiguration gnc = new GridNodeConfiguration();
+    new JCommander(gnc, args);
+    assertEquals("dummyhost", gnc.getHubHost());
+  }
+
+  @Test
+  public void testOneOfHubOrHubHostShouldBePresent() {
+    final String[] args = new String[]{"-hubPort", "1234"};
+    GridNodeConfiguration gnc = new GridNodeConfiguration();
+    new JCommander(gnc, args);
+    Throwable t = TestUtilities.catchThrowable(gnc::getHubHost);
+    assertThat(t, CoreMatchers.instanceOf(RuntimeException.class));
+    t = TestUtilities.catchThrowable(gnc::getHubPort);
+    assertThat(t, CoreMatchers.instanceOf(RuntimeException.class));
+  }
+
+  @Test
+  public void testHubOptionHasPrecedenceOverHubHost() {
+    final String[] args = new String[]{"-hub", "http://smarthost:4321/wd/hub",
+                                       "-hubHost", "dummyhost", "-hubPort", "1234"};
+    GridNodeConfiguration gnc = new GridNodeConfiguration();
+    new JCommander(gnc, args);
+    assertEquals("smarthost", gnc.getHubHost());
+  }
+
   @Test
   public void testGetHubPort() {
+    final String[] args = new String[]{"-hubHost", "dummyhost", "-hubPort", "1234"};
     GridNodeConfiguration gnc = new GridNodeConfiguration();
-    gnc.hub = "http://dummyhost:4444/wd/hub";
-    assertEquals(4444, gnc.getHubPort().intValue());
+    new JCommander(gnc, args);
+    assertEquals(1234, gnc.getHubPort().intValue());
+  }
+
+  @Test
+  public void testGetHubPortFromHubOption() {
+    final String[] args = new String[]{"-hub", "http://dummyhost:1234/wd/hub"};
+    GridNodeConfiguration gnc = new GridNodeConfiguration();
+    new JCommander(gnc, args);
+    assertEquals(1234, gnc.getHubPort().intValue());
+  }
+
+  @Test
+  public void testOneOfHubOrHubPortShouldBePresent() {
+    final String[] args = new String[]{"-hubHost", "dummyhost"};
+    GridNodeConfiguration gnc = new GridNodeConfiguration();
+    new JCommander(gnc, args);
+    Throwable t = TestUtilities.catchThrowable(gnc::getHubHost);
+    assertThat(t, CoreMatchers.instanceOf(RuntimeException.class));
+    t = TestUtilities.catchThrowable(gnc::getHubPort);
+    assertThat(t, CoreMatchers.instanceOf(RuntimeException.class));
+  }
+
+  @Test
+  public void testHubOptionHasPrecedenceOverHubPort() {
+    final String[] args = new String[]{"-hub", "http://smarthost:4321/wd/hub",
+                                       "-hubHost", "dummyhost", "-hubPort", "1234"};
+    GridNodeConfiguration gnc = new GridNodeConfiguration();
+    new JCommander(gnc, args);
+    assertEquals(4321, gnc.getHubPort().intValue());
   }
 
   @Test