----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56991/#review166581 -----------------------------------------------------------
My main input overall is to create a UnitTest using mocking for every new class. It would be ideal to create UnitTests for every existing class we modify. The only exception is classes that primarily interact with file system or threads -- IntegrationTesting of those is probably sufficient. geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java (line 36) <https://reviews.apache.org/r/56991/#comment238561> Delete empty @return geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java (line 50) <https://reviews.apache.org/r/56991/#comment238560> Delete empty @param tags geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java (line 54) <https://reviews.apache.org/r/56991/#comment238559> Delete empty @return geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java (line 128) <https://reviews.apache.org/r/56991/#comment238562> Add final qualifier geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java (line 761) <https://reviews.apache.org/r/56991/#comment238563> Delete printStackTrace call. The logger.error will print the stack trace. geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java (line 768) <https://reviews.apache.org/r/56991/#comment238564> I realize this is just moved, but I'd go ahead and change to: lopgger.debug("Exporting logs returning = {}", result); geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java (line 797) <https://reviews.apache.org/r/56991/#comment238569> Most of the command functions we inherited do something like this but it loses the stack trace. I think we want to consider using the full stackTrackAsString -- example: result = ResultBuilder.createGemFireErrorResult(CliStrings.SHOW_LOG_ERROR + CliUtil.stackTraceAsString(e)); Better yet use sendException (I marked a later line where you use getResult().sendException() and I think that's the pattern we want to change all command functions to use. geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java (line 1037) <https://reviews.apache.org/r/56991/#comment238570> What's #SB? I know it used to be this string as well, but maybe we should change it? geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java (line 97) <https://reviews.apache.org/r/56991/#comment238571> This is the best syntax for sending an exception back from a Function. Change my earlier recommendations from using CliUtil.stackTraceAsString to using sendException instead. geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java (line 243) <https://reviews.apache.org/r/56991/#comment238572> Replace with logger.error(e); geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java (line 282) <https://reviews.apache.org/r/56991/#comment238573> Preferred syntax is: if (Path.class.isInstance(response)) { (I don't think it matters which syntax we use tho) geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ExportLogsCacheWriter.java (line 31) <https://reviews.apache.org/r/56991/#comment238574> As we write new classes (or even edit old classes), we really need to be disciplines about creating true unit tests that mock every collaborator. I think being test driven is more important than pairing. This changes the design of classes and gets us away from having to rely so much on end-to-end tests. geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ExportLogsRepository.java (line 21) <https://reviews.apache.org/r/56991/#comment238576> Should this be an Interface instead of a Class? This seems wrong. At a minimum, I'd expect it to be abstract, but I would delete "Map exportFiles" since this class doesn't use it. The highest subclass that uses this var should define it. geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java (line 46) <https://reviews.apache.org/r/56991/#comment238577> Classes like this don't lend well to mocking for a unit test so it's ok for this to be an exception to the overall recommendation of being unit test driven. Lowest level test would be an integration test since this interacts with files. geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterTest.java (line 100) <https://reviews.apache.org/r/56991/#comment238579> Delete empty test geode-core/src/test/java/org/apache/geode/management/internal/cli/util/MergeLogsTest.java (line 42) <https://reviews.apache.org/r/56991/#comment238580> Rename to MergeLogsDistributedTest or MergeLogsDUnitTest geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpDunit.java (line 45) <https://reviews.apache.org/r/56991/#comment238581> Rename to ExportLogsOverHttpDunitTest - Kirk Lund On Feb. 23, 2017, 6:01 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56991/ > ----------------------------------------------------------- > > (Updated Feb. 23, 2017, 6:01 p.m.) > > > Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, and Kirk > Lund. > > > Repository: geode > > > Description > ------- > > I am putting this up for review for now. It's a big changeset which includes > the following JIRA tickets: basically it's using replicated region as a > communication channel to transfer logs from remote servers to the > jmx-manager/locator that initiates this command, and then gather all the logs > as a http response to send it back to the gfsh client: > > GEODE-2414: Determine a mechanism to stream a zip file from server to locator > GEODE-2415: Write a function to return a zip file for a single server > GEODE-2414: Create exportLogs region for all locators > GEODE-2418: enable gfsh to download file from http connection > > > Diffs > ----- > > geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java > a20fba5a1b9fa185283bd7c60c4ade77345ed4ac > > geode-core/src/main/java/org/apache/geode/management/internal/cli/AbstractCliAroundInterceptor.java > de24727819856d180a69ed0021c21aa016e71712 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CliAroundInterceptor.java > 11d74c13311923357e82318b32bf0342e156e0c6 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CliUtil.java > 8525b5862274cbaa98e57455869d404465851291 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandRequest.java > b0242c9e8ee656001cf76155f4e727ece07666a2 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java > bbf0542b80469e11d21a2ed56f52ef9e647e91ea > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > c536e8e99ef32043d9caf267dd563ee2d05b6e50 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java > b53c53f91ed63a01bfaa1232d9e194481ae45664 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ExportLogsCacheWriter.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ExportLogsRepository.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogFilter.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogLevelExtractor.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/MergeLogs.java > 8d2ef4599337cb091da6f26c6034b1a71311148d > > geode-core/src/main/java/org/apache/geode/management/internal/configuration/utils/ZipUtils.java > 81161d6f3e2539ca3d09765e99b891d1522da302 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MiscellaneousCommandsController.java > ac912c82c873c443dd268f07e8874b5bd18fdd0b > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java > fa052489772e3e03eb865d17dbbcb7e227813c42 > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvoker.java > 82b2b1fc0d2c8952835b0101c413161d39f361dc > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/SimpleHttpOperationInvoker.java > dcda27bef61bad1d7caf1fe1a99063f99d768819 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandRequestTest.java > 543623766b3422d63a1c3ceca38229144c94076b > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnit.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommandsExportLogsPart1DUnitTest.java > 2fc8e844d319abcf19430cfb9cd09e2a64aab38c > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommandsExportLogsPart2DUnitTest.java > d1f44d131bb6c4abe944d5bfd319ebe267b2f7b2 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommandsExportLogsPart3DUnitTest.java > b6a4a5ba3bce03cae8c46442cb032ec270455ed9 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommandsExportLogsPart4DUnitTest.java > b712785b74e4f65fbfedbb362a129151e34c18bb > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunctionIntegrationTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogFilterTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogLevelExtractorTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/MergeLogsTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/EventTestCacheWriter.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ZipUtilsJUnitTest.java > 1791574bc688774900ba2e609f3c80600cb5cac9 > > geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java > 2d32905d45496fe69e5dc189cd94b82ea163c3b5 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java > 4729be3f4d9b51168422784a57ac1ec76e018e83 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarterRule.java > c3e493e1c176974ed757c71d5c195ff5964dbb57 > geode-core/src/test/java/org/apache/geode/test/dunit/rules/Member.java > 7cc1eea2e26b176b570111bccdb8914e19528ecf > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > b5ddee666c52fe39deb015c6e2aa57e5b21e4f20 > > geode-core/src/test/resources/org/apache/geode/codeAnalysis/sanctionedSerializables.txt > 8c0fb4569d0fbfa99622a18bac41281b1cc561df > > geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties > 694d4ffd2c01b0e99010fa425fcbeac08029843b > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpDUnitTest.java > d4869c2110f69275c566eb52e5d09a38ebe30589 > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpDunit.java > PRE-CREATION > > geode-web/src/test/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvokerJUnitTest.java > 808c78f0f4a39501563a389e8b156452c48ffe81 > > geode-web/src/test/java/org/apache/geode/management/internal/web/shell/SimpleHttpOperationInvokerJUnitTest.java > 8afbcf683824545dff050d88ef320a260855c056 > > Diff: https://reviews.apache.org/r/56991/diff/ > > > Testing > ------- > > precheckin running > > > Thanks, > > Jinmei Liao > >
