SOLR-18107: Test fix testLogLevelHandlerOutput#4146
SOLR-18107: Test fix testLogLevelHandlerOutput#4146dsmiley wants to merge 1 commit intoapache:mainfrom
Conversation
LoggingHandlerTest.testLogLevelHandlerOutput Use hard references for consistent logger lookup success
|
I'll commit in 48 hours if no review |
There was a problem hiding this comment.
Pull request overview
Fixes flakiness in LoggingHandlerTest.testLogLevelHandlerOutput by ensuring logger instances are strongly referenced so Log4j2’s weakly-referenced logger cache doesn’t allow loggers to be GC’d and recreated mid-test.
Changes:
- Add strong
Loggerreferences for the relevant logger names used by the test. - Update “effective level” assertions to use the held logger instances instead of repeated lookups.
- Minor comment text adjustment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| { // sanity check our setup... | ||
|
|
||
| // did anybody break the anotations? |
There was a problem hiding this comment.
Typo in comment: "anotations" should be "annotations".
| // did anybody break the anotations? | |
| // did anybody break the annotations? |
| assertEquals(Level.DEBUG, LogManager.getLogger(A_LOGGER_NAME).getLevel()); | ||
| assertEquals(Level.TRACE, LogManager.getLogger(B_LOGGER_NAME).getLevel()); | ||
| assertEquals(Level.TRACE, LogManager.getLogger(BX_LOGGER_NAME).getLevel()); | ||
| // ...and with its effective values |
There was a problem hiding this comment.
Minor grammar issue in the comment: "...and with its effective values" reads awkwardly and seems to be missing a verb. Consider rephrasing to "...and what its effective values are" (or similar) for clarity.
| // ...and with its effective values | |
| // ...and what its effective values are |
| // check directly with log4j what its (updated) config has... | ||
| assertEquals(Level.DEBUG, config.getLoggerConfig(PARENT_LOGGER_NAME).getExplicitLevel()); |
There was a problem hiding this comment.
This block corrects "it's" → "its" here, but a similar comment later in the UNSET section still uses "what it's (updated) config has...". Please update that occurrence too to keep wording consistent.
| // Hold strong references to prevent GC from removing loggers during test | ||
| private Logger parentLogger; | ||
| private Logger aLogger; | ||
| private Logger bLogger; | ||
| private Logger bxLogger; | ||
|
|
There was a problem hiding this comment.
These logger references are only used within testLogLevelHandlerOutput(). Consider making them local final variables inside the test method instead of instance fields; local variables already keep strong references for the whole method execution and avoid introducing shared mutable state in the test class.
1 participant
LoggingHandlerTest.testLogLevelHandlerOutput
Use hard references for consistent logger lookup success
https://issues.apache.org/jira/browse/SOLR-18107