----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58050/#review170497 -----------------------------------------------------------
Every single class that is touched needs to have a new UnitTest written for it. If none exists, then create a new UnitTest "MyClassTest" with @Category(UnitTest.class). We should never touch a class going forward without doing this first. geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java Line 182 (original), 182 (patched) <https://reviews.apache.org/r/58050/#comment243335> Please start using tests to drive what you're doing. Example: try adding a new test to GfshParserJUnitTest which is going to cause one of these Exceptions to be thrown. As you start practicing TDD, you'll end up writing tests for everything and before you make any product code changes. You'll also find that you in many cases you'll need to start refactoring the class in order to facilitate proper testing (unit testing). geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java Line 53 (original), 53 (patched) <https://reviews.apache.org/r/58050/#comment243336> I keep seeing people changing this class and my reviews keep asking for the same things: 1) rename it to ExportLogsCommand to be consistent with other code, 2) create ExportLogsCommandTest which is a UnitTest and start writing some unit tests for this class. - Kirk Lund On March 29, 2017, 10:33 p.m., Patrick Rhomberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58050/ > ----------------------------------------------------------- > > (Updated March 29, 2017, 10:33 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, > Kirk Lund, and Swapnil Bawaskar. > > > Repository: geode > > > Description > ------- > > Also: Collapsed unrelated exception catch blocks. > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java > a1d03e45dd4d6559bd9a0869c7dd95908d1858ca > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java > 3f147c19a128dce78c51c31e6758e517cd2ab496 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java > d74f5d6b4fce9a44d9eeebdfc7dcf716e9b1652b > > > Diff: https://reviews.apache.org/r/58050/diff/1/ > > > Testing > ------- > > Manual verification of behavior both with and without --dir option, both > connected via HTTP and not. > > I should write some unit tests for it, but would like to pair on that since > these tests would be more involved than a copy-paste-modify of the other unit > test I "wrote." > > Question: do we prefer to be consistent with previously existing behavior, or > consistent in behavior across connection types? That initial conditional is > only in place to preserve existing behavior, where the non-HTTP connection > defaults to placing logs in the locator's directory. Without that block, we > would move logs to the current working directory, which would be consistent > with the via-HTTP scenario. > > > Thanks, > > Patrick Rhomberg > >
