----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60348/#review178703 -----------------------------------------------------------
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java Lines 103 (patched) <https://reviews.apache.org/r/60348/#comment252855> I've always disliked writing loops with offsets from the loop counter used within the loop body, they make you pause and reason out whether the index is correct. I think something like ``` int i=1; for (String cmd:specifiedCommands) { commandsToExecute[i++] = "-e " + cmd; } ``` reads a bit better, although it does separate the loop iterator and the index counter. geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java Line 56 (original), 59 (patched) <https://reviews.apache.org/r/60348/#comment252860> Probably should only add the enclosing '"' on `value` if the string doesn't already have them. A quick grep showed 3 tests (IndexCOmmandsDUnitTest) that have enclosing quotes on the value arg. -OR- change those tests to be consistent with adding the quotes here. - Ken Howe On June 21, 2017, 10:51 p.m., Jared Stewart wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60348/ > ----------------------------------------------------------- > > (Updated June 21, 2017, 10:51 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > ------- > > - GfshRule writes via a logger rather than StdOut. This will make it no > longer clutter precheckin runs or the nightly build. > - Introduce ProcessLogger to capture output from the Gfsh JVM so that tests > can assert against the output. > > > Diffs > ----- > > > geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java > 82ee240539b0190a8698939faa63c696d1e03efb > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshExecution.java > PRE-CREATION > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java > 81093778d379d6610b94ee93b180bcc773eb3595 > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshScript.java > 3ee1402aa9c390d1bc097e29190010ff1496fca6 > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/ProcessLogger.java > PRE-CREATION > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/StreamGobbler.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java > cfb8a24d250af11723c27d35112f6a5e4fde4568 > > > Diff: https://reviews.apache.org/r/60348/diff/1/ > > > Testing > ------- > > Precheckin running > > > Thanks, > > Jared Stewart > >
