Move all option processing on boot into one place.

This isn't particularly elegant, and there's a lot of duplication, but this means as we slide the new config into place we don't need to hunt down quite so much stuff. It's deeply unfortunate that we expose jcommander. My hope is that once we clean everything up, we can go back and hide our use of jcommander once and for all.
SeleniumHQ · Aug 9, 2018 · 6ca14aa · 6ca14aa
1 parent 7c63cea
commit 6ca14aa
Show file tree

Hide file tree

Showing 13 changed files with 137 additions and 81 deletions.
diff --git a/java/server/src/org/openqa/grid/internal/cli/GridHubCliOptions.java b/java/server/src/org/openqa/grid/internal/cli/GridHubCliOptions.java
@@ -26,8 +26,10 @@
 
 public class GridHubCliOptions extends CommonGridCliOptions {
 
-  public GridHubCliOptions(String[] args) {
-    JCommander.newBuilder().addObject(this).build().parse(args);
+  public JCommander parse(String... args) {
+    JCommander commander = JCommander.newBuilder().addObject(this).build();
+    commander.parse(args);
+    return commander;
   }
 
   /**

diff --git a/java/server/src/org/openqa/grid/internal/cli/GridNodeCliOptions.java b/java/server/src/org/openqa/grid/internal/cli/GridNodeCliOptions.java
@@ -27,8 +27,10 @@
 
 public class GridNodeCliOptions extends CommonGridCliOptions {
 
-  public GridNodeCliOptions(String[] args) {
-    JCommander.newBuilder().addObject(this).build().parse(args);
+  public JCommander parse(String... args) {
+    JCommander commander = JCommander.newBuilder().addObject(this).build();
+    commander.parse(args);
+    return commander;
   }
 
   /**

diff --git a/java/server/src/org/openqa/grid/internal/cli/StandaloneCliOptions.java b/java/server/src/org/openqa/grid/internal/cli/StandaloneCliOptions.java
@@ -22,13 +22,10 @@
 
 public class StandaloneCliOptions extends CommonCliOptions {
 
-  public StandaloneCliOptions(String[] args) {
-    JCommander.newBuilder().addObject(this).build().parse(args);
-  }
-
-  public StandaloneCliOptions parse(String[] args) {
-    JCommander.newBuilder().addObject(this).build().parse(args);
-    return this;
+  public JCommander parse(String... args) {
+    JCommander commander = JCommander.newBuilder().addObject(this).build();
+    commander.parse(args);
+    return commander;
   }
 
   /**

diff --git a/java/server/src/org/openqa/grid/selenium/GridLauncherV3.java b/java/server/src/org/openqa/grid/selenium/GridLauncherV3.java
@@ -24,7 +24,6 @@
 import com.beust.jcommander.JCommander;
 
 import org.openqa.grid.common.GridRole;
-import org.openqa.grid.internal.cli.CommonCliOptions;
 import org.openqa.grid.internal.cli.GridHubCliOptions;
 import org.openqa.grid.internal.cli.GridNodeCliOptions;
 import org.openqa.grid.internal.cli.StandaloneCliOptions;
@@ -48,6 +47,8 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
+import java.util.function.BiConsumer;
+import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.logging.ConsoleHandler;
 import java.util.logging.FileHandler;
@@ -65,14 +66,9 @@ public class GridLauncherV3 {
   private PrintStream out;
   private String[] args;
 
+  @FunctionalInterface
   private interface GridItemLauncher {
-    CommonCliOptions getOptions();
-    Stoppable launch() throws Exception;
-    default void printUsage(PrintStream out) {
-      StringBuilder sb = new StringBuilder();
-      new JCommander(getOptions()).usage(sb);
-      out.print(sb);
-    }
+    Optional<Stoppable> launch(PrintStream out) throws Exception;
   }
 
   private static Map<String, Function<String[], GridItemLauncher>> LAUNCHERS = buildLaunchers();
@@ -100,29 +96,10 @@ public Optional<Stoppable> launch() {
       return Optional.empty();
     }
 
-    if (launcher.getOptions().help) {
-      launcher.printUsage(out);
-      return Optional.empty();
-    }
-
-    if (launcher.getOptions().version) {
-      out.println(String.format("Selenium server version: %s, revision: %s",
-                                buildInfo.getReleaseLabel(), buildInfo.getBuildRevision()));
-      return Optional.empty();
-    }
-
-    configureLogging(launcher.getOptions().getLog(), launcher.getOptions().getDebug());
-
-    log.info(String.format(
-        "Selenium build info: version: '%s', revision: '%s'",
-        buildInfo.getReleaseLabel(),
-        buildInfo.getBuildRevision()));
     try {
-      return Optional.of(launcher.launch());
+      return launcher.launch(out);
     } catch (Exception e) {
-      launcher.printUsage(out);
-      e.printStackTrace();
-      return Optional.empty();
+      throw new RuntimeException(e);
     }
   }
 
@@ -244,18 +221,44 @@ private static void configureLogging(String log, boolean debug) {
   }
 
   private static Map<String, Function<String[], GridItemLauncher>> buildLaunchers() {
+    BiConsumer<JCommander, PrintStream> usage = (commander, out) -> {
+      StringBuilder toPrint = new StringBuilder();
+      commander.usage(toPrint);
+      out.append(toPrint);
+    };
+
+    Consumer<PrintStream> version = out -> {
+      out.println(String.format(
+          "Selenium server version: %s, revision: %s",
+          buildInfo.getReleaseLabel(),
+          buildInfo.getBuildRevision()));
+    };
+
     ImmutableMap.Builder<String, Function<String[], GridItemLauncher>> launchers =
       ImmutableMap.<String, Function<String[], GridItemLauncher>>builder()
         .put(GridRole.NOT_GRID.toString(), (args) -> new GridItemLauncher() {
-          StandaloneCliOptions options = new StandaloneCliOptions(args);
-
           @Override
-          public CommonCliOptions getOptions() {
-            return options;
-          }
+          public Optional<Stoppable> launch(PrintStream out) {
+            StandaloneCliOptions options = new StandaloneCliOptions();
+            JCommander commander = options.parse(args);
+
+            if (options.getVersion()) {
+              version.accept(out);
+              return Optional.empty();
+            }
+
+            if (options.getHelp()) {
+              usage.accept(commander, out);
+              return Optional.empty();
+            }
+
+            configureLogging(options.getLog(), options.getDebug());
+
+            log.info(String.format(
+                "Selenium build info: version: '%s', revision: '%s'",
+                buildInfo.getReleaseLabel(),
+                buildInfo.getBuildRevision()));
 
-          @Override
-          public Stoppable launch() {
             StandaloneConfiguration configuration = new StandaloneConfiguration(options);
             log.info(String.format(
                 "Launching a standalone Selenium Server on port %s", configuration.port));
@@ -264,38 +267,65 @@ public Stoppable launch() {
             servlets.put("/*", DisplayHelpServlet.class);
             server.setExtraServlets(servlets);
             server.boot();
-            return server;
+            return Optional.of(server);
           }
-
         })
         .put(GridRole.HUB.toString(), (args) -> new GridItemLauncher() {
-          GridHubCliOptions options = new GridHubCliOptions(args);
 
           @Override
-          public CommonCliOptions getOptions() {
-            return options;
-          }
+          public Optional<Stoppable> launch(PrintStream out) throws Exception {
+            GridHubCliOptions options = new GridHubCliOptions();
+            JCommander commander = options.parse(args);
+
+            if (options.getVersion()) {
+              version.accept(out);
+              return Optional.empty();
+            }
+
+            if (options.getHelp()) {
+              usage.accept(commander, out);
+              return Optional.empty();
+            }
+
+            configureLogging(options.getLog(), options.getDebug());
+
+            log.info(String.format(
+                "Selenium build info: version: '%s', revision: '%s'",
+                buildInfo.getReleaseLabel(),
+                buildInfo.getBuildRevision()));
 
-          @Override
-          public Stoppable launch() throws Exception {
             GridHubConfiguration configuration = new GridHubConfiguration(options);
             log.info(String.format(
                 "Launching Selenium Grid hub on port %s", configuration.port));
             Hub hub = new Hub(configuration);
             hub.start();
-            return hub;
+            return Optional.of(hub);
           }
         })
         .put(GridRole.NODE.toString(), (args) -> new GridItemLauncher() {
-          GridNodeCliOptions options = new GridNodeCliOptions(args);
 
           @Override
-          public CommonCliOptions getOptions() {
-            return options;
-          }
+          public Optional<Stoppable> launch(PrintStream out) throws Exception {
+            GridNodeCliOptions options = new GridNodeCliOptions();
+            JCommander commander = options.parse(args);
+
+            if (options.getVersion()) {
+              version.accept(out);
+              return Optional.empty();
+            }
+
+            if (options.getHelp()) {
+              usage.accept(commander, out);
+              return Optional.empty();
+            }
+
+            configureLogging(options.getLog(), options.getDebug());
+
+            log.info(String.format(
+                "Selenium build info: version: '%s', revision: '%s'",
+                buildInfo.getReleaseLabel(),
+                buildInfo.getBuildRevision()));
 
-          @Override
-          public Stoppable launch() throws Exception {
             GridNodeConfiguration configuration = new GridNodeConfiguration(options);
             if (configuration.port == null || configuration.port == -1) {
               configuration.port = PortProber.findFreePort();
@@ -309,7 +339,7 @@ public Stoppable launch() throws Exception {
               log.info("Selenium Grid node is up and ready to register to the hub");
               remote.startRegistrationProcess();
             }
-            return server;
+            return Optional.of(server);
           }
         });
 

diff --git a/java/server/src/org/openqa/selenium/remote/server/SeleniumServer.java b/java/server/src/org/openqa/selenium/remote/server/SeleniumServer.java
@@ -292,8 +292,9 @@ private void stopAllBrowsers() {
     }
   }
 
-  public static void main(String[] argv) {
-    StandaloneCliOptions options = new StandaloneCliOptions(argv);
+  public static void main(String[] args) {
+    StandaloneCliOptions options = new StandaloneCliOptions();
+    options.parse(args);
 
     if (options.help) {
       StringBuilder message = new StringBuilder();
@@ -310,7 +311,9 @@ public static void usage(String msg) {
     if (msg != null) {
       System.out.println(msg);
     }
-    JCommander jCommander = new JCommander(new StandaloneCliOptions(new String[]{}));
-    jCommander.usage();
+
+    StandaloneCliOptions options = new StandaloneCliOptions();
+    JCommander commander = options.parse();
+    commander.usage();
   }
 }
diff --git a/java/server/test/org/openqa/grid/common/RegistrationRequestTest.java b/java/server/test/org/openqa/grid/common/RegistrationRequestTest.java
@@ -376,7 +376,8 @@ private void assertConstruction(RegistrationRequest req) {
   }
 
   private GridNodeConfiguration parseCliOptions(String... args) {
-    GridNodeCliOptions opts = new GridNodeCliOptions(args);
+    GridNodeCliOptions opts = new GridNodeCliOptions();
+    opts.parse(args);
     return new GridNodeConfiguration(opts);
   }
 }
diff --git a/java/server/test/org/openqa/grid/e2e/utils/GridTestHelper.java b/java/server/test/org/openqa/grid/e2e/utils/GridTestHelper.java
@@ -48,7 +48,9 @@ public static SelfRegisteringRemote getRemoteWithoutCapabilities(URL hub, GridRo
                      "-host","localhost",
                      "-hub",hub.toString(),
                      "-port",String.valueOf(PortProber.findFreePort())};
-    GridNodeConfiguration config = new GridNodeConfiguration(new GridNodeCliOptions(args));
+    GridNodeCliOptions options = new GridNodeCliOptions();
+    options.parse(args);
+    GridNodeConfiguration config = new GridNodeConfiguration(options);
     RegistrationRequest req = RegistrationRequest.build(config);
     SelfRegisteringRemote remote = new SelfRegisteringRemote(req);
     remote.deleteAllBrowsers();

diff --git a/java/server/test/org/openqa/grid/internal/BaseRemoteProxyTest.java b/java/server/test/org/openqa/grid/internal/BaseRemoteProxyTest.java
@@ -189,6 +189,8 @@ public void teardown() {
   }
 
   private GridNodeConfiguration parseCliOptions(String... args) {
-    return new GridNodeConfiguration(new GridNodeCliOptions(args));
+    GridNodeCliOptions options = new GridNodeCliOptions();
+    options.parse(args);
+    return new GridNodeConfiguration(options);
   }
 }
diff --git a/java/server/test/org/openqa/grid/internal/UserDefinedCapabilityMatcherTests.java b/java/server/test/org/openqa/grid/internal/UserDefinedCapabilityMatcherTests.java
@@ -33,7 +33,9 @@ public class UserDefinedCapabilityMatcherTests {
   public void defaultsToDefaultMatcher() {
     GridRegistry registry = DefaultGridRegistry.newInstance(new Hub(new GridHubConfiguration()));
     String[] args = new String[]{"-role", "webdriver","-id", "abc","-host","localhost"};
-    GridNodeConfiguration nodeConfiguration = new GridNodeConfiguration(new GridNodeCliOptions(args));
+    GridNodeCliOptions options = new GridNodeCliOptions();
+    options.parse(args);
+    GridNodeConfiguration nodeConfiguration = new GridNodeConfiguration(options);
     RegistrationRequest req = RegistrationRequest.build(nodeConfiguration);
     req.getConfiguration().proxy = null;
     RemoteProxy p = BaseRemoteProxy.getNewInstance(req, registry);
@@ -49,7 +51,9 @@ public void capabilityMatcherCanBeSpecified() {
     hubConfig.capabilityMatcher = new MyCapabilityMatcher();
     Hub hub = new Hub(hubConfig);
     String[] args = new String[]{"-role", "webdriver","-id", "abc","-host","localhost"};
-    GridNodeConfiguration nodeConfiguration = new GridNodeConfiguration(new GridNodeCliOptions(args));
+    GridNodeCliOptions options = new GridNodeCliOptions();
+    options.parse(args);
+    GridNodeConfiguration nodeConfiguration = new GridNodeConfiguration(options);
     RegistrationRequest req = RegistrationRequest.build(nodeConfiguration);
     req.getConfiguration().proxy = null;
     RemoteProxy p = BaseRemoteProxy.getNewInstance(req, hub.getRegistry());

diff --git a/java/server/test/org/openqa/grid/internal/utils/configuration/GridHubConfigurationTest.java b/java/server/test/org/openqa/grid/internal/utils/configuration/GridHubConfigurationTest.java
@@ -64,7 +64,7 @@ public void testDefaultsFromConfig() {
 
   @Test
   public void testDefaultsFromCli() {
-    GridHubCliOptions cliConfig = new GridHubCliOptions(new String[]{});
+    GridHubCliOptions cliConfig = new GridHubCliOptions();
     checkDefaults(new GridHubConfiguration(cliConfig));
   }
 
@@ -174,7 +174,9 @@ public void testToString() {
     ghc = new GridHubConfiguration();
     String[] args = ("-servlet com.foo.bar.ServletA -servlet com.foo.bar.ServletB"
                      + " -custom foo=bar,bar=baz").split(" ");
-    ghc = new GridHubConfiguration(new GridHubCliOptions(args));
+    GridHubCliOptions options = new GridHubCliOptions();
+    options.parse(args);
+    ghc = new GridHubConfiguration(options);
 
     assertTrue(ghc.toString().contains("-servlets com.foo.bar.ServletA"
                                        + " -servlets com.foo.bar.ServletB"));
@@ -187,7 +189,9 @@ public void testToString() {
   public void testJcommanderConverterCapabilityMatcher() {
     String[] hubArgs = {"-capabilityMatcher", "org.openqa.grid.internal.utils.DefaultCapabilityMatcher",
                         "-prioritizer", "org.openqa.grid.internal.utils.configuration.PlaceHolderTestingPrioritizer"};
-    GridHubConfiguration ghc = new GridHubConfiguration(new GridHubCliOptions(hubArgs));
+    GridHubCliOptions options = new GridHubCliOptions();
+    options.parse(hubArgs);
+    GridHubConfiguration ghc = new GridHubConfiguration(options);
     assertEquals("org.openqa.grid.internal.utils.DefaultCapabilityMatcher",
                  ghc.capabilityMatcher.getClass().getCanonicalName());
     assertEquals("org.openqa.grid.internal.utils.configuration.PlaceHolderTestingPrioritizer",

diff --git a/java/server/test/org/openqa/grid/internal/utils/configuration/GridNodeConfigurationTest.java b/java/server/test/org/openqa/grid/internal/utils/configuration/GridNodeConfigurationTest.java
@@ -77,7 +77,7 @@ public void testConstructorEqualsDefaultConfig() {
 
   @Test
   public void testDefaultsFromCli() {
-    checkDefaults(new GridNodeConfiguration(new GridNodeCliOptions(new String[]{})));
+    checkDefaults(new GridNodeConfiguration(new GridNodeCliOptions()));
   }
 
   private void checkDefaults(GridNodeConfiguration gnc) {
@@ -203,7 +203,10 @@ public void testAsJson() {
   public void testWithCapabilitiesArgs() {
     final String[] args = new String[] { "-capabilities",
                                        "browserName=chrome,platform=linux,maxInstances=10,boolean=false" };
-    GridNodeConfiguration gnc = new GridNodeConfiguration(new GridNodeCliOptions(args));
+
+    GridNodeCliOptions options = new GridNodeCliOptions();
+    options.parse(args);
+    GridNodeConfiguration gnc = new GridNodeConfiguration(options);
     assertTrue(gnc.capabilities.size() == 1);
     assertEquals("chrome", gnc.capabilities.get(0).getBrowserName());
     assertEquals(10L, gnc.capabilities.get(0).getCapability("maxInstances"));
@@ -347,7 +350,9 @@ public void hubOptionHasPrecedenceOverNodeConfig() throws IOException {
   }
 
   private GridNodeConfiguration parseCliOptions(String... args) {
-    return new GridNodeConfiguration(new GridNodeCliOptions(args));
+    GridNodeCliOptions options = new GridNodeCliOptions();
+    options.parse(args);
+    return new GridNodeConfiguration(options);
   }
 
   @Test