KAFKA-18834: Fix LoggingResourceTest#testSetLevelDefaultScope

[apalan60]

Jira: KAFKA-18834

• Flaky behavior
  LoggingResourceTest#testSetLevelDefaultScope sometimes fails by not capturing its expected WARN log.

• Root cause
  Both LoggersTest#testSetLevelWithValidRootLoggerNames and LoggingResourceTest#testSetLevelDefaultScope may share the same LoggerContext when executed in the same JVM.
  LoggersTest#testSetLevelWithValidRootLoggerNames calls loggers.setLevel("", ERROR), which mutates the global root logger level to ERROR and suppresses WARN logs—causing subsequent tests to fail to emit WARN-level output.

• Fix in this PR
  The original root level is saved before each test in LoggersTest and restored after the test completes.

[gemini-code-assist]

Hello @apalan60, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello! Gemini here, providing a summary of this pull request. This PR addresses a flaky test, LoggingResourceTest#testSetLevelDefaultScope, which occasionally failed because it didn't capture an expected WARN log message. The root cause was identified as another test, LoggersTest#testSetLevelWithValidRootLoggerNames, modifying the global root logger level to ERROR without restoring it. When these tests ran in the same JVM, the LoggersTest would inadvertently suppress WARN logs for subsequent tests like LoggingResourceTest. The fix implemented in this PR is to modify LoggersTest to save the original root logger level before each test run and restore it in an @AfterEach method, ensuring that the global logger state is reset between tests and preventing interference.

Highlights

• Fix Flaky Test: Resolves the flakiness in LoggingResourceTest#testSetLevelDefaultScope by preventing interference from other tests that modify global logger state.
• Improve Test Isolation: Enhances the isolation of tests within LoggersTest by ensuring that changes to the global root logger level are cleaned up after each test method executes.
• Logging Configuration Management in Tests: Demonstrates proper handling of shared, mutable logging configuration (root logger level) within JUnit tests using @BeforeEach and @AfterEach.

Changelog

• connect/runtime/src/test/java/org/apache/kafka/connect/runtime/LoggersTest.java
  • Added necessary imports for Log4j Level, LogManager, Configurator, and JUnit AfterEach (lines 23-27).
  • Added a field originalRootLevel to store the root logger's level before each test (line 48).
  • Modified the @BeforeEach method setup() to save the current root logger level into originalRootLevel (line 52).
  • Added a new @AfterEach method teardown() to restore the root logger level to the value saved in originalRootLevel (lines 57-60).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request effectively addresses the flaky test LoggingResourceTest#testSetLevelDefaultScope by ensuring that LoggersTest restores the global root logger level after its tests complete. The approach of saving the original level in @BeforeEach and restoring it in @AfterEach is a standard and appropriate way to manage shared state in tests, preventing side effects on other tests.

The changes are clear, concise, and directly target the identified root cause. Well done on diagnosing and fixing this tricky issue!

Summary of Findings

• Field Visibility: The field originalRootLevel in LoggersTest.java has default (package-private) visibility. While acceptable in test classes, making it private would slightly improve encapsulation as it's only used within LoggersTest. This is a low-severity stylistic suggestion and was not commented on directly due to review settings.

Merge Readiness

The changes look good and effectively address the described test flakiness. The solution is robust for the problem at hand. I believe this PR is in good shape to be merged. As always, ensure any CI checks pass and consider if further review by others is standard practice for your team. I am not authorized to approve the pull request.

[apalan60]

/gemini summary

[gemini-code-assist]

Summary of Changes

Hello! Gemini has reviewed this pull request. The main purpose of this change is to address a flaky test, specifically LoggingResourceTest#testSetLevelDefaultScope. The flakiness was identified as being caused by another test, LoggersTest#testSetLevelWithValidRootLoggerNames, which modifies the global root logger level during its execution and doesn't reset it afterwards. When these tests run in the same JVM and share the same LoggerContext, the change made by LoggersTest can suppress expected log messages in subsequent tests like LoggingResourceTest, causing them to fail intermittently. This PR fixes this by modifying LoggersTest to save the original root logger level before each test runs and restore it in a teardown method, ensuring that its logging level changes do not affect other tests.

Highlights

• Fix Flaky Test: Resolves the intermittent failure of LoggingResourceTest#testSetLevelDefaultScope.
• Improve Test Isolation: Ensures that logging level changes made within LoggersTest do not leak and affect other tests running in the same JVM by saving and restoring the global root logger level.
• Logging Configuration in Tests: Adds setup and teardown logic to manage the state of the root logger level during test execution.

Changelog

• connect/runtime/src/test/java/org/apache/kafka/connect/runtime/LoggersTest.java
  • Added imports for Level, LogManager, Configurator, and AfterEach to support saving and restoring logging configuration.
  • Introduced a field originalRootLevel to store the root logger level before a test runs.
  • Implemented a @BeforeEach method (setup) to capture the current root logger level using LogManager.getRootLogger().getLevel() (added around line 52).
  • Implemented an @AfterEach method (teardown) to restore the root logger level to its original state using Configurator.setAllLevels(LogManager.ROOT_LOGGER_NAME, originalRootLevel) (added around line 59).

Activity

• The author, @apalan60, requested this summary.
• There are no other review comments or activity on this pull request yet.

[apalan60]

@CodiumAI-Agent /review

[QodoAI-Agent]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Test Isolation Restoring only the root logger's level may not fully isolate test side effects, especially if other tests modify logger configurations or child loggers. Consider using a separate LoggerContext or resetting the complete logging configuration. Level originalRootLevel; @BeforeEach public void setup() { originalRootLevel = LogManager.getRootLogger().getLevel(); time = new MockTime(0, INITIAL_TIME, 0); loggers = (Loggers.Log4jLoggers) Loggers.newInstance(time); } @AfterEach public void teardown() { Configurator.setAllLevels(LogManager.ROOT_LOGGER_NAME, originalRootLevel); } Implementation Coupling The test directly uses Configurator from log4j-core, coupling the tests to Log4j 2 implementation details. Consider using a higher-level logging API or abstractions to reduce tight coupling. import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.core.config.Configurator; import org.junit.jupiter.api.AfterEach;