----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58010/#review170493 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java Line 49 (original), 49 (patched) <https://reviews.apache.org/r/58010/#comment243328> Move this constant into ExportLogCommand class geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java Line 156 (original), 156 (patched) <https://reviews.apache.org/r/58010/#comment243330> I recommend creating a new ExportLogsFunctionArgsTest which is a UnitTest for this ExportLogsFunction.Args class. At a minimum you create a test which confirms the default log level: ```java @Test public void defaultLogLevelShouldBeAll() throws Exception { ... assertThat(logLeve).isEqualTo(ALL); } ``` Better yet, use this as practice for TDD. Remove your log level change and write this test so it fails BEFORE changing the default log level. Then change the default so the test passes. Define any additional tests that you think are valuable (use "", use null, use all defaults). Don't go crazy with tons of tests just some basic tests. Any class worth having is worth having a basic unit test for it. geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java Line 156 (original), 156 (patched) <https://reviews.apache.org/r/58010/#comment243331> - Kirk Lund On March 28, 2017, 10:34 p.m., Patrick Rhomberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58010/ > ----------------------------------------------------------- > > (Updated March 28, 2017, 10:34 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, > Kirk Lund, and Swapnil Bawaskar. > > > Repository: geode > > > Description > ------- > > Added DUnit test that fails under previous behavior. > > > Diffs > ----- > > geode-core/src/main/java/org/apache/geode/internal/logging/LogService.java > 1f8a564a8111b036f5e5cce6393931c714985c9c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > cbdf1c4bc28554a8fbec3740c566ee07c69b4ac9 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java > 95edd426da8b8f39bb1486661d8c307d43f170d6 > > > Diff: https://reviews.apache.org/r/58010/diff/1/ > > > Testing > ------- > > Added DUnit passes under new behavior. > > === > > While `LogService.DEFAULT_LOG_LEVEL` is only used by the export logs command, > I don't know if that is the best place to make this update. Should that stay > `INFO` and a new variable `DEFAULT_EXPORT_LOG_LEVEL` be established? > > > Thanks, > > Patrick Rhomberg > >
