From d4672834754dd33a9a44fd8fdba58bdcbc42b08e Mon Sep 17 00:00:00 2001 From: Andrew Ross Date: Fri, 19 Nov 2021 12:37:46 -0600 Subject: [PATCH] Start MockLogAppender before adding to static context (#1587) I observed a test failure with the message 'Attempted to append to non-started appender mock' from an assertion in `OpenSearchTestCase::after`. I believe this indicates that a MockLogAppender (which is named "mock") was added as an appender to the static logging context and some other test in the same JVM happened to cause a logging statement to hit that appender and cause an error, which then caused an unrelated test to fail (because they share static state with the logger). Almost all usages of MockLogAppender start it immediately after creation. I found a few that did not and fixed those. I also made a static helper in MockLogAppender to start it upon creation. Signed-off-by: Andrew Ross --- .../netty4/OpenSearchLoggingHandlerIT.java | 3 +-- .../transport/nio/NioTransportLoggingIT.java | 3 +-- .../common/settings/SettingsFilterTests.java | 3 +-- .../opensearch/transport/TransportLoggerTests.java | 3 +-- .../java/org/opensearch/test/MockLogAppender.java | 14 +++++++++++++- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/netty4/OpenSearchLoggingHandlerIT.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/netty4/OpenSearchLoggingHandlerIT.java index 90006ef78bb..d7703390cb3 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/netty4/OpenSearchLoggingHandlerIT.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/netty4/OpenSearchLoggingHandlerIT.java @@ -53,11 +53,10 @@ public class OpenSearchLoggingHandlerIT extends OpenSearchNetty4IntegTestCase { public void setUp() throws Exception { super.setUp(); - appender = new MockLogAppender(); + appender = MockLogAppender.createStarted(); Loggers.addAppender(LogManager.getLogger(OpenSearchLoggingHandler.class), appender); Loggers.addAppender(LogManager.getLogger(TransportLogger.class), appender); Loggers.addAppender(LogManager.getLogger(TcpTransport.class), appender); - appender.start(); } public void tearDown() throws Exception { diff --git a/plugins/transport-nio/src/internalClusterTest/java/org/opensearch/transport/nio/NioTransportLoggingIT.java b/plugins/transport-nio/src/internalClusterTest/java/org/opensearch/transport/nio/NioTransportLoggingIT.java index 4c8e711be33..64c6edc48c3 100644 --- a/plugins/transport-nio/src/internalClusterTest/java/org/opensearch/transport/nio/NioTransportLoggingIT.java +++ b/plugins/transport-nio/src/internalClusterTest/java/org/opensearch/transport/nio/NioTransportLoggingIT.java @@ -54,10 +54,9 @@ public class NioTransportLoggingIT extends NioIntegTestCase { public void setUp() throws Exception { super.setUp(); - appender = new MockLogAppender(); + appender = MockLogAppender.createStarted(); Loggers.addAppender(LogManager.getLogger(TransportLogger.class), appender); Loggers.addAppender(LogManager.getLogger(TcpTransport.class), appender); - appender.start(); } public void tearDown() throws Exception { diff --git a/server/src/test/java/org/opensearch/common/settings/SettingsFilterTests.java b/server/src/test/java/org/opensearch/common/settings/SettingsFilterTests.java index 993aa45cc91..1278489f18d 100644 --- a/server/src/test/java/org/opensearch/common/settings/SettingsFilterTests.java +++ b/server/src/test/java/org/opensearch/common/settings/SettingsFilterTests.java @@ -143,10 +143,9 @@ public class SettingsFilterTests extends OpenSearchTestCase { private void assertExpectedLogMessages(Consumer consumer, MockLogAppender.LoggingExpectation... expectations) throws IllegalAccessException { Logger testLogger = LogManager.getLogger("org.opensearch.test"); - MockLogAppender appender = new MockLogAppender(); + MockLogAppender appender = MockLogAppender.createStarted(); Loggers.addAppender(testLogger, appender); try { - appender.start(); Arrays.stream(expectations).forEach(appender::addExpectation); consumer.accept(testLogger); appender.assertAllExpectationsMatched(); diff --git a/server/src/test/java/org/opensearch/transport/TransportLoggerTests.java b/server/src/test/java/org/opensearch/transport/TransportLoggerTests.java index ad8538a5e37..12cc7be4b1e 100644 --- a/server/src/test/java/org/opensearch/transport/TransportLoggerTests.java +++ b/server/src/test/java/org/opensearch/transport/TransportLoggerTests.java @@ -56,9 +56,8 @@ public class TransportLoggerTests extends OpenSearchTestCase { public void setUp() throws Exception { super.setUp(); - appender = new MockLogAppender(); + appender = MockLogAppender.createStarted(); Loggers.addAppender(LogManager.getLogger(TransportLogger.class), appender); - appender.start(); } public void tearDown() throws Exception { diff --git a/test/framework/src/main/java/org/opensearch/test/MockLogAppender.java b/test/framework/src/main/java/org/opensearch/test/MockLogAppender.java index cf810772302..df42a6f0237 100644 --- a/test/framework/src/main/java/org/opensearch/test/MockLogAppender.java +++ b/test/framework/src/main/java/org/opensearch/test/MockLogAppender.java @@ -51,7 +51,19 @@ public class MockLogAppender extends AbstractAppender { private static final String COMMON_PREFIX = System.getProperty("opensearch.logger.prefix", "org.opensearch."); - private List expectations; + private final List expectations; + + /** + * Creates and starts a MockLogAppender. Generally preferred over using the constructor + * directly because adding an unstarted appender to the static logging context can cause + * difficult-to-identify errors in the tests and this method makes it impossible to do + * that. + */ + public static MockLogAppender createStarted() throws IllegalAccessException { + final MockLogAppender appender = new MockLogAppender(); + appender.start(); + return appender; + } public MockLogAppender() throws IllegalAccessException { super("mock", RegexFilter.createFilter(".*(\n.*)*", new String[0], false, null, null), null);