Re: Review Request 36957: GEODE-181: Add new FastLogger tests and fix isDebug/TraceEnabled checks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36957/#review93977 --- Ship it! Ship It! - Darrel Schneider On Aug. 3, 2015, 1:34 p.m., Kirk Lund wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36957/ --- (Updated Aug. 3, 2015, 1:34 p.m.) Review request for geode and Darrel Schneider. Bugs: GEODE-181 and GEODE-89 https://issues.apache.org/jira/browse/GEODE-181 https://issues.apache.org/jira/browse/GEODE-89 Repository: geode Description --- I found GEODE-181 while refactoring the tests in FastLoggerJUnitTest for GEODE-89. These changes address both tickets. Extract testDefaultConfig to FastLoggerWithDefaultConfigJUnitTest. Refactor remaining test in FastLoggerJUnitTest to improve coverage and readability. Introduce use of TemporaryFolder instead of using tmp dir. Change from UnitTest to IntegrationTest because of file system I/O and execution speed. Add new TestSuite classes for targeted testing of logging and log4j packages. Diffs - build.gradle c82e82a gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/LogService.java 6298cf6 gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/Configurator.java c7ae945 gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/FastLogger.java 21d7965 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java 9c7ba58 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingUnitTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerIntegrationJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerJUnitTest.java 2aab5df gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerWithDefaultConfigJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jUnitTestSuite.java PRE-CREATION Diff: https://reviews.apache.org/r/36957/diff/ Testing --- test, integrationTest, new TestSuites, rebuild open+closed Thanks, Kirk Lund
Re: Review Request 36957: GEODE-181: Add new FastLogger tests and fix isDebug/TraceEnabled checks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36957/#review93956 --- gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/LogService.java (line 87) https://reviews.apache.org/r/36957/#comment148399 This PropertyChangeListener is invokes this method once and then this line was invoking it a second time. I'll delete this line. gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/LogService.java (line 150) https://reviews.apache.org/r/36957/#comment148401 I'll delete all deadcode such as these lines. gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/LogService.java (line 256) https://reviews.apache.org/r/36957/#comment148402 I discovered that only two filter types were being handled correctly. Appender and AppenderRef filters were not being handled correctly. Basically, if there are any filters defined or if the level is DEBUG or lower then FastLogger must delegate to the underlying logger rather than early-out from the isEnabled checks. gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/FastLogger.java (line 21) https://reviews.apache.org/r/36957/#comment148404 I renamed this to delegating to avoid confusion with isDebugEnabled which is a pair of methods on Logger API. gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerIntegrationJUnitTest.java (line 37) https://reviews.apache.org/r/36957/#comment148408 Moved most tests from FastLoggerJUnitTest to here because they were all IntegrationTests. Expanded coverage to include all log4j2 filter types and all boundary/transition points interesting for FastLogger (ex: to/and/from isDelegating). gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerWithDefaultConfigJUnitTest.java (line 56) https://reviews.apache.org/r/36957/#comment148407 Need to rename debugAvailable to delegating - Kirk Lund On Aug. 3, 2015, 7:58 p.m., Kirk Lund wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36957/ --- (Updated Aug. 3, 2015, 7:58 p.m.) Review request for geode and Darrel Schneider. Bugs: GEODE-181 and GEODE-89 https://issues.apache.org/jira/browse/GEODE-181 https://issues.apache.org/jira/browse/GEODE-89 Repository: geode Description --- I found GEODE-181 while refactoring the tests in FastLoggerJUnitTest for GEODE-89. These changes address both tickets. Extract testDefaultConfig to FastLoggerWithDefaultConfigJUnitTest. Refactor remaining test in FastLoggerJUnitTest to improve coverage and readability. Introduce use of TemporaryFolder instead of using tmp dir. Change from UnitTest to IntegrationTest because of file system I/O and execution speed. Add new TestSuite classes for targeted testing of logging and log4j packages. Diffs - build.gradle c82e82a gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/LogService.java 6298cf6 gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/Configurator.java c7ae945 gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/FastLogger.java 21d7965 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java 9c7ba58 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingUnitTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerIntegrationJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerJUnitTest.java 2aab5df gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerWithDefaultConfigJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jUnitTestSuite.java PRE-CREATION Diff: https://reviews.apache.org/r/36957/diff/ Testing --- test, integrationTest, new TestSuites, rebuild open+closed Thanks, Kirk Lund
Re: Review Request 36957: GEODE-181: Add new FastLogger tests and fix isDebug/TraceEnabled checks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36957/ --- (Updated Aug. 3, 2015, 8:32 p.m.) Review request for geode and Darrel Schneider. Bugs: GEODE-181 and GEODE-89 https://issues.apache.org/jira/browse/GEODE-181 https://issues.apache.org/jira/browse/GEODE-89 Repository: geode Description --- I found GEODE-181 while refactoring the tests in FastLoggerJUnitTest for GEODE-89. These changes address both tickets. Extract testDefaultConfig to FastLoggerWithDefaultConfigJUnitTest. Refactor remaining test in FastLoggerJUnitTest to improve coverage and readability. Introduce use of TemporaryFolder instead of using tmp dir. Change from UnitTest to IntegrationTest because of file system I/O and execution speed. Add new TestSuite classes for targeted testing of logging and log4j packages. Diffs (updated) - build.gradle c82e82a gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/LogService.java 6298cf6 gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/Configurator.java c7ae945 gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/FastLogger.java 21d7965 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java 9c7ba58 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingUnitTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerIntegrationJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerJUnitTest.java 2aab5df gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerWithDefaultConfigJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jUnitTestSuite.java PRE-CREATION Diff: https://reviews.apache.org/r/36957/diff/ Testing --- test, integrationTest, new TestSuites, rebuild open+closed Thanks, Kirk Lund
Re: Review Request 36957: GEODE-181: Add new FastLogger tests and fix isDebug/TraceEnabled checks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36957/ --- (Updated Aug. 3, 2015, 7:58 p.m.) Review request for geode and Darrel Schneider. Changes --- Created new FastLogger unit tests and integration tests. Added build dependency on mockito (GEODE-104). Bugs: GEODE-181 and GEODE-89 https://issues.apache.org/jira/browse/GEODE-181 https://issues.apache.org/jira/browse/GEODE-89 Repository: geode Description --- I found GEODE-181 while refactoring the tests in FastLoggerJUnitTest for GEODE-89. These changes address both tickets. Extract testDefaultConfig to FastLoggerWithDefaultConfigJUnitTest. Refactor remaining test in FastLoggerJUnitTest to improve coverage and readability. Introduce use of TemporaryFolder instead of using tmp dir. Change from UnitTest to IntegrationTest because of file system I/O and execution speed. Add new TestSuite classes for targeted testing of logging and log4j packages. Diffs (updated) - build.gradle c82e82a gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/LogService.java 6298cf6 gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/Configurator.java c7ae945 gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/FastLogger.java 21d7965 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java 9c7ba58 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingUnitTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerIntegrationJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerJUnitTest.java 2aab5df gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerWithDefaultConfigJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jUnitTestSuite.java PRE-CREATION Diff: https://reviews.apache.org/r/36957/diff/ Testing --- test, integrationTest, new TestSuites, rebuild open+closed Thanks, Kirk Lund
Re: Review Request 36957: GEODE-181: Add new FastLogger tests and fix isDebug/TraceEnabled checks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36957/ --- (Updated Aug. 3, 2015, 8:34 p.m.) Review request for geode and Darrel Schneider. Changes --- Latest complete diffs Bugs: GEODE-181 and GEODE-89 https://issues.apache.org/jira/browse/GEODE-181 https://issues.apache.org/jira/browse/GEODE-89 Repository: geode Description --- I found GEODE-181 while refactoring the tests in FastLoggerJUnitTest for GEODE-89. These changes address both tickets. Extract testDefaultConfig to FastLoggerWithDefaultConfigJUnitTest. Refactor remaining test in FastLoggerJUnitTest to improve coverage and readability. Introduce use of TemporaryFolder instead of using tmp dir. Change from UnitTest to IntegrationTest because of file system I/O and execution speed. Add new TestSuite classes for targeted testing of logging and log4j packages. Diffs (updated) - build.gradle c82e82a gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/LogService.java 6298cf6 gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/Configurator.java c7ae945 gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/FastLogger.java 21d7965 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/DistributedSystemLogFileJUnitTest.java 9c7ba58 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingUnitTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerIntegrationJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerJUnitTest.java 2aab5df gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerWithDefaultConfigJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jUnitTestSuite.java PRE-CREATION Diff: https://reviews.apache.org/r/36957/diff/ Testing --- test, integrationTest, new TestSuites, rebuild open+closed Thanks, Kirk Lund
Review Request 36957: GEODE-181: Add new FastLogger tests and fix isDebug/TraceEnabled checks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36957/ --- Review request for geode and Darrel Schneider. Bugs: GEODE-181 and GEODE-89 https://issues.apache.org/jira/browse/GEODE-181 https://issues.apache.org/jira/browse/GEODE-89 Repository: geode Description --- I found GEODE-181 while refactoring the tests in FastLoggerJUnitTest for GEODE-89. These changes address both tickets. Extract testDefaultConfig to FastLoggerWithDefaultConfigJUnitTest. Refactor remaining test in FastLoggerJUnitTest to improve coverage and readability. Introduce use of TemporaryFolder instead of using tmp dir. Change from UnitTest to IntegrationTest because of file system I/O and execution speed. Add new TestSuite classes for targeted testing of logging and log4j packages. Diffs - gemfire-core/src/main/java/com/gemstone/gemfire/internal/logging/log4j/FastLogger.java 21d7965 gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/LoggingUnitTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerJUnitTest.java 2aab5df gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/FastLoggerWithDefaultConfigJUnitTest.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jIntegrationTestSuite.java PRE-CREATION gemfire-core/src/test/java/com/gemstone/gemfire/internal/logging/log4j/Log4jUnitTestSuite.java PRE-CREATION Diff: https://reviews.apache.org/r/36957/diff/ Testing --- test, integrationTest, new TestSuites, rebuild open+closed Thanks, Kirk Lund
