vy commented on code in PR #3263:
URL: https://github.com/apache/logging-log4j2/pull/3263#discussion_r1866548995


##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerMessageFactoryCustomizationTest.java:
##########
@@ -24,34 +24,36 @@
 import org.apache.logging.log4j.message.MessageFactory;
 import org.apache.logging.log4j.message.ParameterizedMessageFactory;
 import org.junit.jupiter.api.Test;
-import org.junitpioneer.jupiter.ClearSystemProperty;
 import org.junitpioneer.jupiter.SetSystemProperty;
 
+@SetSystemProperty(
+        key = "log4j2.messageFactory",
+        value = 
"org.apache.logging.log4j.core.LoggerMessageFactoryCustomizationTest$TestMessageFactory")
+@SetSystemProperty(
+        key = "log4j2.flowMessageFactory",
+        value = 
"org.apache.logging.log4j.core.LoggerMessageFactoryCustomizationTest$TestFlowMessageFactory")
 class LoggerMessageFactoryCustomizationTest {
 
     @Test
-    @ClearSystemProperty(key = "log4j2.messageFactory")
-    @ClearSystemProperty(key = "log4j2.flowMessageFactory")
     void arguments_should_be_honored() {
-        final LoggerContext loggerContext =
-                new 
LoggerContext(LoggerMessageFactoryCustomizationTest.class.getSimpleName());
-        final Logger logger = new Logger(
-                loggerContext, "arguments_should_be_honored", new 
TestMessageFactory(), new TestFlowMessageFactory());
-        assertTestMessageFactories(logger);
+        try (LoggerContext loggerContext =
+                new 
LoggerContext(LoggerMessageFactoryCustomizationTest.class.getSimpleName())) {
+            Logger logger = new Logger(
+                    loggerContext,
+                    "arguments_should_be_honored",

Review Comment:
   You can use `@TestInfo` instead of hardcoding the method name. (Same goes 
for the other test methods too.)



##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerMessageFactoryCustomizationTest.java:
##########
@@ -24,34 +24,36 @@
 import org.apache.logging.log4j.message.MessageFactory;
 import org.apache.logging.log4j.message.ParameterizedMessageFactory;
 import org.junit.jupiter.api.Test;
-import org.junitpioneer.jupiter.ClearSystemProperty;
 import org.junitpioneer.jupiter.SetSystemProperty;
 
+@SetSystemProperty(

Review Comment:
   By making this test-scoped, aren't you changing the 
`arguments_should_be_honored()` test? That is,
   
   * Before: there are no property-provided configurations, but only arguments 
passed to `Logger::new`
   * New: there are property-provided configurations, and also arguments passed 
to `Logger::new`
   
   In the new case, you cannot know which configuration source has kicked in.



##########
log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilTest.java:
##########
@@ -208,6 +208,28 @@ void testBadPropertySource() {
         }
     }
 
+    @Test
+    @ResourceLock(value = Resources.SYSTEM_PROPERTIES, mode = 
ResourceAccessMode.READ)
+    @Issue("https://github.com/apache/logging-log4j2/issues/3252";)
+    void testRecursivePropertySource() {

Review Comment:
   Shall we also verify the status logger warning?



-- 
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: notifications-unsubscr...@logging.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to