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 <andrross@amazon.com>

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 {

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 {

server/src/test/java/org/opensearch/common/settings/SettingsFilterTests.java

@@ -143,10 +143,9 @@ public class SettingsFilterTests extends OpenSearchTestCase {
     private void assertExpectedLogMessages(Consumer<Logger> 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();

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 {

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<LoggingExpectation> expectations;
+    private final List<LoggingExpectation> 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);