Skip to content

Commit

Permalink
Fix log appender in InsecureSettingTests (#11866)
Browse files Browse the repository at this point in the history
The appender was being added to the logger before the appender was
started. This can lead to logger errors with concurrently running tests
because the logger is static state shared across the JVM. See #10799.

I've also added a removeAppender call that was missing from
LoggersTests, but that is mostly hygiene and would not lead to failures.

Signed-off-by: Andrew Ross <[email protected]>
(cherry picked from commit c8ae7f0)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
  • Loading branch information
github-actions[bot] committed Jan 12, 2024
1 parent cfbff9f commit f0e2ee5
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -53,40 +53,45 @@ public void testParameterizedMessageLambda() throws Exception {
appender.start();
final Logger testLogger = LogManager.getLogger(LoggersTests.class);
Loggers.addAppender(testLogger, appender);
Loggers.setLevel(testLogger, Level.TRACE);
try {
Loggers.setLevel(testLogger, Level.TRACE);

Throwable ex = randomException();
testLogger.error(() -> new ParameterizedMessage("an error message"), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.ERROR));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an error message"));
Throwable ex = randomException();
testLogger.error(() -> new ParameterizedMessage("an error message"), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.ERROR));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an error message"));

ex = randomException();
testLogger.warn(() -> new ParameterizedMessage("a warn message: [{}]", "long gc"), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.WARN));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a warn message: [long gc]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining("long gc"));
ex = randomException();
testLogger.warn(() -> new ParameterizedMessage("a warn message: [{}]", "long gc"), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.WARN));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a warn message: [long gc]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining("long gc"));

testLogger.info(() -> new ParameterizedMessage("an info message a=[{}], b=[{}], c=[{}]", 1, 2, 3));
assertThat(appender.lastEvent.getLevel(), equalTo(Level.INFO));
assertThat(appender.lastEvent.getThrown(), nullValue());
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an info message a=[1], b=[2], c=[3]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(1, 2, 3));
testLogger.info(() -> new ParameterizedMessage("an info message a=[{}], b=[{}], c=[{}]", 1, 2, 3));
assertThat(appender.lastEvent.getLevel(), equalTo(Level.INFO));
assertThat(appender.lastEvent.getThrown(), nullValue());
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an info message a=[1], b=[2], c=[3]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(1, 2, 3));

ex = randomException();
testLogger.debug(() -> new ParameterizedMessage("a debug message options = {}", Arrays.asList("yes", "no")), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.DEBUG));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a debug message options = [yes, no]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(Arrays.asList("yes", "no")));
ex = randomException();
testLogger.debug(() -> new ParameterizedMessage("a debug message options = {}", Arrays.asList("yes", "no")), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.DEBUG));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a debug message options = [yes, no]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(Arrays.asList("yes", "no")));

ex = randomException();
testLogger.trace(() -> new ParameterizedMessage("a trace message; element = [{}]", new Object[] { null }), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.TRACE));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a trace message; element = [null]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(new Object[] { null }));
ex = randomException();
testLogger.trace(() -> new ParameterizedMessage("a trace message; element = [{}]", new Object[] { null }), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.TRACE));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a trace message; element = [null]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(new Object[] { null }));
} finally {
Loggers.removeAppender(testLogger, appender);
appender.stop();
}
}

private Throwable randomException() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public class InsecureSettingTests extends OpenSearchTestCase {
private List<String> rootLogMsgs = new ArrayList<>();
private AbstractAppender rootAppender;

protected void assertSettingWarning() {
private void assertSettingWarning() {
assertWarnings(
"[setting.name] setting was deprecated in OpenSearch and will be removed in a future release! See the breaking changes documentation for the next major version."
);
Expand All @@ -50,13 +50,14 @@ public void append(LogEvent event) {
InsecureSettingTests.this.rootLogMsgs.add(message);
}
};
Loggers.addAppender(LogManager.getRootLogger(), rootAppender);
rootAppender.start();
Loggers.addAppender(LogManager.getLogger(SecureSetting.class), rootAppender);
}

@After
public void removeInsecureSettingsAppender() {
Loggers.removeAppender(LogManager.getRootLogger(), rootAppender);
Loggers.removeAppender(LogManager.getLogger(SecureSetting.class), rootAppender);
rootAppender.stop();
}

public void testShouldRaiseExceptionByDefault() {
Expand Down

0 comments on commit f0e2ee5

Please sign in to comment.