----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57514/#review168676 -----------------------------------------------------------
I really need to see UnitTests or IntegrationTests being written. In particular, we need to be focused on using Mockito to create new UnitTests. I'm available to pair on this if there are any questions about how to do this. I really do not want to see any of these classes being changed further without new tests being written. geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java Line 53 (original), 53 (patched) <https://reviews.apache.org/r/57514/#comment240952> Since the command is "export logs" can we please rename this class to ExportLogsCommand? Then we really need to create ExportLogsCommandTest (UnitTest) and/or ExportLogsCommandIntegrationTest (IntegrationTest). I don't think we should be altering any code that we don't create a new UnitTest and/or IntegrationTest for. To make this class testable with Mocks, you would for example, not invoke GemFireCacheImpl.getInstance() inside exportLogs. Instead create: private GemFireCache gemfireCache; Set this in a new constructor: public ExportLogsCommand() { this.gemfireCache = GemFireCacheImpl.getInstance(); } Now you can actually test this class in a UnitTest with Mockito by simply using @Mock annotation on a private GemFireCache variable in the test and Mockito will inject that mock into this class! geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java Line 37 (original), 40 (patched) <https://reviews.apache.org/r/57514/#comment240953> This is another class without UnitTest or IntegrationTest. Expecting the existing high-level DistributedTest (dunit) to cover these changes is not what we want. We want to pay down technical debt as we go and that means taking the time to create better unit tests for any new classes (this should be done 100% without question) and also for old classes that we need to modify. So I'm looking for either ExportLogControllerTest or ExportLogControllerIntegrationTest or both. If the class needs to touch file system or sockets for basic testing then it's ok for that to be an ExportLogControllerIntegrationTest and you can even use Mockito in that type of testing. geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java Line 85 (original), 85 (patched) <https://reviews.apache.org/r/57514/#comment240954> Another class that we really need to create a UnitTest or IntegrationTest for. - Kirk Lund On March 10, 2017, 5:17 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57514/ > ----------------------------------------------------------- > > (Updated March 10, 2017, 5:17 p.m.) > > > Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, > and Patrick Rhomberg. > > > Repository: geode > > > Description > ------- > > * correctly output error message if gfsh execution has an error > * export logs should output correct log message over http connection as well > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java > 36d071c153ed8c2559cc8c29e248058b42cbf7ff > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java > ad78efd34fc4c9b1598751f71ba58ff5d29476a4 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java > 0351573f6eda80e1751bd6b95efdbe6ce36a620d > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java > f60aabceeae5d973caa900e5443b7cec0c0a75ca > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportStatsDUnitTest.java > b7f8c2a080614ab59dc9da29c31f606749b58e4a > > > Diff: https://reviews.apache.org/r/57514/diff/1/ > > > Testing > ------- > > precheckin running ... > > > Thanks, > > Jinmei Liao > >
