Copilot commented on code in PR #4146:
URL: https://github.com/apache/solr/pull/4146#discussion_r2825839255
##########
solr/core/src/test/org/apache/solr/handler/admin/LoggingHandlerTest.java:
##########
@@ -130,18 +144,18 @@ public void testLogLevelHandlerOutput() throws Exception {
assertLoggerLevel(updatedLoggerLevel, B_LOGGER_NAME, "TRACE", true);
assertLoggerLevel(updatedLoggerLevel, BX_LOGGER_NAME, "TRACE", false);
- // check directly with log4j what it's (updated) config has...
+ // check directly with log4j what its (updated) config has...
assertEquals(Level.DEBUG,
config.getLoggerConfig(PARENT_LOGGER_NAME).getExplicitLevel());
assertEquals(Level.TRACE,
config.getLoggerConfig(B_LOGGER_NAME).getExplicitLevel());
assertEquals(
"Unexpected config for BX ... expected B's config",
config.getLoggerConfig(B_LOGGER_NAME),
config.getLoggerConfig(BX_LOGGER_NAME));
- // ...and what it's effective values
- assertEquals(Level.DEBUG,
LogManager.getLogger(PARENT_LOGGER_NAME).getLevel());
- 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
Review 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.
```suggestion
// ...and what its effective values are
```
##########
solr/core/src/test/org/apache/solr/handler/admin/LoggingHandlerTest.java:
##########
@@ -130,18 +144,18 @@ public void testLogLevelHandlerOutput() throws Exception {
assertLoggerLevel(updatedLoggerLevel, B_LOGGER_NAME, "TRACE", true);
assertLoggerLevel(updatedLoggerLevel, BX_LOGGER_NAME, "TRACE", false);
- // check directly with log4j what it's (updated) config has...
+ // check directly with log4j what its (updated) config has...
assertEquals(Level.DEBUG,
config.getLoggerConfig(PARENT_LOGGER_NAME).getExplicitLevel());
Review 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.
##########
solr/core/src/test/org/apache/solr/handler/admin/LoggingHandlerTest.java:
##########
@@ -66,6 +73,13 @@ public void testLogLevelHandlerOutput() throws Exception {
final Configuration config = ctx.getConfiguration();
+ // Create and hold strong references to loggers to prevent GC from
removing them
+ // during test execution (Log4j2 uses weak references internally)
+ parentLogger = LogManager.getLogger(PARENT_LOGGER_NAME);
+ aLogger = LogManager.getLogger(A_LOGGER_NAME);
+ bLogger = LogManager.getLogger(B_LOGGER_NAME);
+ bxLogger = LogManager.getLogger(BX_LOGGER_NAME);
+
{ // sanity check our setup...
// did anybody break the anotations?
Review Comment:
Typo in comment: "anotations" should be "annotations".
```suggestion
// did anybody break the annotations?
```
##########
solr/core/src/test/org/apache/solr/handler/admin/LoggingHandlerTest.java:
##########
@@ -46,6 +47,12 @@ public class LoggingHandlerTest extends SolrTestCaseJ4 {
private final String B_LOGGER_NAME = PARENT_LOGGER_NAME + ".BogusClass_B";
private final String BX_LOGGER_NAME = B_LOGGER_NAME + ".BogusNestedClass_X";
+ // Hold strong references to prevent GC from removing loggers during test
+ private Logger parentLogger;
+ private Logger aLogger;
+ private Logger bLogger;
+ private Logger bxLogger;
+
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
