Skip to content

Commit

Permalink
Fix log appender in InsecureSettingTests (opensearch-project#11866) (o…
Browse files Browse the repository at this point in the history 
…pensearch-project#11879)

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 opensearch-project#10799.

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


(cherry picked from commit c8ae7f0)

Signed-off-by: Andrew Ross <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
  • Loading branch information
@opensearch-trigger-bot @github-actions
opensearch-trigger-bot[bot] and github-actions[bot] authored Jan 13, 2024
1 parent cfbff9f commit e93425d
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 32 deletions.
63 changes: 34 additions & 29 deletions server/src/test/java/org/opensearch/common/logging/LoggersTests.java
View file
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
7 changes: 4 additions & 3 deletions server/src/test/java/org/opensearch/common/settings/InsecureSettingTests.java
View file
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 e93425d

Please sign in to comment.