----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59736/#review176899 -----------------------------------------------------------
It does seem lots of added boilerplate code. And we need to repeat that for start server as well unless we put the common code in a parent class. the complexity seems to be big. To unit test a single command without having to pass in all these null values, I think we might be able to use GfshParseResult to do this. I cooked up a GfshParserRule in this PR: https://github.com/apache/geode/pull/557. Perhaps you can see if you can use it. See LauncherLifeCycleCommandTest for example. - Jinmei Liao On June 1, 2017, 11:10 p.m., Jared Stewart wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59736/ > ----------------------------------------------------------- > > (Updated June 1, 2017, 11:10 p.m.) > > > Review request for geode. > > > Repository: geode > > > Description > ------- > > I'm having trouble finding a way to test the actual product changes for > GEODE-2933 that doesn't feel hacky. I don't intend to commit this review > request in the current state. Rather, I wanted to start a discussion to > validate the general strategy and consider alternative suggestions before I > get too far into the work. > > There is a StartLocatorCommandArguments class introduced that makes easier > passing around all of the arguments to startLocator(). It also allows setting > one or two arguments for tests without needing to put the other 20 nulls in > the correct positions. Thoughts/questions I still have: > > 1) Does the improved testability justify the added boilerplate of > StartLocatorCommandArguments? > 2) Should the setting of "default values" (lines ~221-246 in > LauncherLifecycleCommands) also be moved into this class? > 3) It seems like there some overlap between the intent of > StartLocatorCommandArguments and LocatorLauncher.Builder - not sure where the > boundary between these two should be. > 4) There are more places that `args` could replace telescoped methods. E.g. > `createStartLocatorCommandLine` could take in `(locatorLauncher, args)` > rather than ```(locatorLauncher, > gemfirePropertiesPathname, gemfireSecurityPropertiesPathname, > gemfireProperties, > classpath, includeSystemClasspath, jvmArgsOpts, initialHeap, > maxHeap)``` > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java > 4c668b6813de71aae9e3e46c82a5c231a41c1f6a > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/lifecycle/StartLocatorCommandArguments.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > 9f68d3a5eaadbe8f1bd95ec8df85ed1f65aa67ce > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/StartLocatorCommandArgumentsTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59736/diff/1/ > > > Testing > ------- > > > Thanks, > > Jared Stewart > >
