timoninmaxim commented on code in PR #11615:
URL: https://github.com/apache/ignite/pull/11615#discussion_r1836102934
##########
modules/core/src/test/java/org/apache/ignite/logger/java/JavaLoggerTest.java:
##########
@@ -17,39 +17,110 @@
package org.apache.ignite.logger.java;
+import java.io.File;
import java.util.UUID;
+import java.util.logging.LogManager;
import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.internal.logger.IgniteLoggerEx;
import org.apache.ignite.internal.util.typedef.internal.U;
import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.apache.ignite.testframework.LogListener;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
import org.apache.ignite.testframework.junits.common.GridCommonTest;
import org.junit.Test;
-import static org.junit.Assert.assertTrue;
+import static org.apache.ignite.logger.java.JavaLogger.DFLT_CONFIG_PATH;
/**
* Java logger test.
*/
@GridCommonTest(group = "Logger")
-public class JavaLoggerTest {
- /** */
- @SuppressWarnings({"FieldCanBeLocal"})
- private IgniteLogger log;
+public class JavaLoggerTest extends GridCommonAbstractTest {
+ /**
+ * Path to jul configuration with DEBUG enabled.
+ */
+ private static final String LOG_CONFIG_DEBUG =
"modules/core/src/test/config/jul-debug.properties";
+
+ /**
+ * Reset JavaLogger.
+ */
+ @Override protected void afterTest() throws Exception {
+ GridTestUtils.setFieldValue(JavaLogger.class, JavaLogger.class,
"inited", false);
+ }
+
Review Comment:
Remove excess blank line
##########
modules/core/src/main/java/org/apache/ignite/logger/java/JavaLogger.java:
##########
@@ -214,13 +205,26 @@ public JavaLogger(final Logger impl, boolean configure) {
quiet = quiet0;
}
+ /**
+ * Creates new logger with given parameters.
+ *
+ * @param impl Java Logging implementation to use.
+ * @param quiet Quiet mode.
+ * @param cfg Path to configuration.
+ */
+ private JavaLogger(Logger impl, boolean quiet, String cfg) {
+ this.impl = impl;
+ this.quiet = quiet;
+ this.cfg = cfg;
+ }
+
/** {@inheritDoc} */
@Override public IgniteLogger getLogger(Object ctgr) {
return new JavaLogger(ctgr == null
? Logger.getLogger("")
: Logger.getLogger(ctgr instanceof Class
? ((Class<?>)ctgr).getName()
- : String.valueOf(ctgr)));
+ : String.valueOf(ctgr)), quiet, cfg);
Review Comment:
It starts invoking another constructor. Before your change there was a chain:
```JavaLogger#getLogger(Object) -> new JavaLogger(Logger, configure=true)```
After your change, in case root logger created as `new JavaLogger(false)`
the underlying logger will use constructor with no configuration and it never
been configured. And the constructor `JavaLogger(Logger, boolean)` is useless.
Is it ok?
##########
modules/core/src/test/java/org/apache/ignite/logger/java/JavaLoggerTest.java:
##########
@@ -17,39 +17,110 @@
package org.apache.ignite.logger.java;
+import java.io.File;
import java.util.UUID;
+import java.util.logging.LogManager;
import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.internal.logger.IgniteLoggerEx;
import org.apache.ignite.internal.util.typedef.internal.U;
import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.apache.ignite.testframework.LogListener;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
import org.apache.ignite.testframework.junits.common.GridCommonTest;
import org.junit.Test;
-import static org.junit.Assert.assertTrue;
+import static org.apache.ignite.logger.java.JavaLogger.DFLT_CONFIG_PATH;
/**
* Java logger test.
*/
@GridCommonTest(group = "Logger")
-public class JavaLoggerTest {
- /** */
- @SuppressWarnings({"FieldCanBeLocal"})
- private IgniteLogger log;
+public class JavaLoggerTest extends GridCommonAbstractTest {
+ /**
+ * Path to jul configuration with DEBUG enabled.
+ */
+ private static final String LOG_CONFIG_DEBUG =
"modules/core/src/test/config/jul-debug.properties";
+
+ /**
+ * Reset JavaLogger.
+ */
+ @Override protected void afterTest() throws Exception {
+ GridTestUtils.setFieldValue(JavaLogger.class, JavaLogger.class,
"inited", false);
+ }
+
+
+ /**
+ * Check JavaLogger default constructor.
+ */
+ @Test
+ public void testDefaultConstructorWithDefaultConfig() {
+ IgniteLogger log1 = new JavaLogger();
+ IgniteLogger log2 = log1.getLogger(getClass());
+
+ assertTrue(log1.toString().contains("JavaLogger"));
+ assertTrue(log1.toString().contains(DFLT_CONFIG_PATH));
+
+ assertTrue(log2.toString().contains("JavaLogger"));
+ assertTrue(log2.toString().contains(DFLT_CONFIG_PATH));
+ }
+
+ /**
+ * Check JavaLogger constructor from java.util.logging.config.file
property.
+ */
+ @Test
+ public void testDefaultConstructorWithProperty() throws Exception {
+ File file = new File(U.getIgniteHome(), LOG_CONFIG_DEBUG);
+ System.setProperty("java.util.logging.config.file", file.getPath());
+ // Call readConfiguration explicitly because Logger.getLogger was
already called during IgniteUtils initialization
+ LogManager.getLogManager().readConfiguration();
+
+ IgniteLogger log1 = new JavaLogger();
+ assertTrue(log1.toString().contains("JavaLogger"));
+ assertTrue(log1.toString().contains(LOG_CONFIG_DEBUG));
+ assertTrue(log1.isDebugEnabled());
+
+ IgniteLogger log2 = log1.getLogger(getClass());
+ assertTrue(log2.toString().contains("JavaLogger"));
+ assertTrue(log2.toString().contains(LOG_CONFIG_DEBUG));
+ assertTrue(log1.isDebugEnabled());
+
+ System.clearProperty("java.util.logging.config.file");
Review Comment:
This code will not call in case of an assertion error.
##########
modules/core/src/test/java/org/apache/ignite/logger/java/JavaLoggerTest.java:
##########
@@ -17,39 +17,110 @@
package org.apache.ignite.logger.java;
+import java.io.File;
import java.util.UUID;
+import java.util.logging.LogManager;
import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.internal.logger.IgniteLoggerEx;
import org.apache.ignite.internal.util.typedef.internal.U;
import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.apache.ignite.testframework.LogListener;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
import org.apache.ignite.testframework.junits.common.GridCommonTest;
import org.junit.Test;
-import static org.junit.Assert.assertTrue;
+import static org.apache.ignite.logger.java.JavaLogger.DFLT_CONFIG_PATH;
/**
* Java logger test.
*/
@GridCommonTest(group = "Logger")
-public class JavaLoggerTest {
- /** */
- @SuppressWarnings({"FieldCanBeLocal"})
- private IgniteLogger log;
+public class JavaLoggerTest extends GridCommonAbstractTest {
+ /**
+ * Path to jul configuration with DEBUG enabled.
+ */
+ private static final String LOG_CONFIG_DEBUG =
"modules/core/src/test/config/jul-debug.properties";
+
+ /**
+ * Reset JavaLogger.
+ */
+ @Override protected void afterTest() throws Exception {
+ GridTestUtils.setFieldValue(JavaLogger.class, JavaLogger.class,
"inited", false);
+ }
+
+
+ /**
+ * Check JavaLogger default constructor.
+ */
+ @Test
+ public void testDefaultConstructorWithDefaultConfig() {
+ IgniteLogger log1 = new JavaLogger();
+ IgniteLogger log2 = log1.getLogger(getClass());
+
+ assertTrue(log1.toString().contains("JavaLogger"));
+ assertTrue(log1.toString().contains(DFLT_CONFIG_PATH));
+
+ assertTrue(log2.toString().contains("JavaLogger"));
+ assertTrue(log2.toString().contains(DFLT_CONFIG_PATH));
+ }
+
+ /**
+ * Check JavaLogger constructor from java.util.logging.config.file
property.
+ */
+ @Test
+ public void testDefaultConstructorWithProperty() throws Exception {
+ File file = new File(U.getIgniteHome(), LOG_CONFIG_DEBUG);
+ System.setProperty("java.util.logging.config.file", file.getPath());
+ // Call readConfiguration explicitly because Logger.getLogger was
already called during IgniteUtils initialization
Review Comment:
Comments should be ended with a dot sign.
##########
modules/core/src/test/java/org/apache/ignite/logger/java/JavaLoggerTest.java:
##########
@@ -17,39 +17,110 @@
package org.apache.ignite.logger.java;
+import java.io.File;
import java.util.UUID;
+import java.util.logging.LogManager;
import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.internal.logger.IgniteLoggerEx;
import org.apache.ignite.internal.util.typedef.internal.U;
import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.apache.ignite.testframework.LogListener;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
import org.apache.ignite.testframework.junits.common.GridCommonTest;
import org.junit.Test;
-import static org.junit.Assert.assertTrue;
+import static org.apache.ignite.logger.java.JavaLogger.DFLT_CONFIG_PATH;
/**
* Java logger test.
*/
@GridCommonTest(group = "Logger")
-public class JavaLoggerTest {
- /** */
- @SuppressWarnings({"FieldCanBeLocal"})
- private IgniteLogger log;
+public class JavaLoggerTest extends GridCommonAbstractTest {
+ /**
+ * Path to jul configuration with DEBUG enabled.
+ */
+ private static final String LOG_CONFIG_DEBUG =
"modules/core/src/test/config/jul-debug.properties";
+
+ /**
+ * Reset JavaLogger.
+ */
+ @Override protected void afterTest() throws Exception {
+ GridTestUtils.setFieldValue(JavaLogger.class, JavaLogger.class,
"inited", false);
+ }
+
+
+ /**
+ * Check JavaLogger default constructor.
+ */
+ @Test
+ public void testDefaultConstructorWithDefaultConfig() {
+ IgniteLogger log1 = new JavaLogger();
+ IgniteLogger log2 = log1.getLogger(getClass());
+
+ assertTrue(log1.toString().contains("JavaLogger"));
+ assertTrue(log1.toString().contains(DFLT_CONFIG_PATH));
+
+ assertTrue(log2.toString().contains("JavaLogger"));
+ assertTrue(log2.toString().contains(DFLT_CONFIG_PATH));
+ }
+
+ /**
+ * Check JavaLogger constructor from java.util.logging.config.file
property.
+ */
+ @Test
+ public void testDefaultConstructorWithProperty() throws Exception {
+ File file = new File(U.getIgniteHome(), LOG_CONFIG_DEBUG);
+ System.setProperty("java.util.logging.config.file", file.getPath());
+ // Call readConfiguration explicitly because Logger.getLogger was
already called during IgniteUtils initialization
+ LogManager.getLogManager().readConfiguration();
+
+ IgniteLogger log1 = new JavaLogger();
+ assertTrue(log1.toString().contains("JavaLogger"));
+ assertTrue(log1.toString().contains(LOG_CONFIG_DEBUG));
+ assertTrue(log1.isDebugEnabled());
+
+ IgniteLogger log2 = log1.getLogger(getClass());
+ assertTrue(log2.toString().contains("JavaLogger"));
+ assertTrue(log2.toString().contains(LOG_CONFIG_DEBUG));
+ assertTrue(log1.isDebugEnabled());
+
+ System.clearProperty("java.util.logging.config.file");
+ }
+
+ /**
+ * Check Grid logging.
+ */
+ @Test
+ public void testGridLoggingWithDefaultLogger() throws Exception {
+ LogListener lsn = LogListener.matches("JavaLogger [quiet=true,")
+ .andMatches(DFLT_CONFIG_PATH)
+ .build();
+ ListeningTestLogger log = new ListeningTestLogger(new JavaLogger(),
lsn);
+
+ IgniteConfiguration cfg =
getConfiguration(getTestIgniteInstanceName());
+ cfg.setGridLogger(log);
+ startGrid(cfg);
+
+ assertTrue(GridTestUtils.waitForCondition(lsn::check, 2_000));
+ stopAllGrids();
Review Comment:
Can be replaced with
```
try(Ignite ign = startGrid(cfg) {
...
}
```
##########
modules/core/src/test/java/org/apache/ignite/logger/java/JavaLoggerTest.java:
##########
@@ -17,39 +17,110 @@
package org.apache.ignite.logger.java;
+import java.io.File;
import java.util.UUID;
+import java.util.logging.LogManager;
import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.internal.logger.IgniteLoggerEx;
import org.apache.ignite.internal.util.typedef.internal.U;
import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.ListeningTestLogger;
+import org.apache.ignite.testframework.LogListener;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
import org.apache.ignite.testframework.junits.common.GridCommonTest;
import org.junit.Test;
-import static org.junit.Assert.assertTrue;
+import static org.apache.ignite.logger.java.JavaLogger.DFLT_CONFIG_PATH;
/**
* Java logger test.
*/
@GridCommonTest(group = "Logger")
-public class JavaLoggerTest {
- /** */
- @SuppressWarnings({"FieldCanBeLocal"})
- private IgniteLogger log;
+public class JavaLoggerTest extends GridCommonAbstractTest {
+ /**
+ * Path to jul configuration with DEBUG enabled.
+ */
+ private static final String LOG_CONFIG_DEBUG =
"modules/core/src/test/config/jul-debug.properties";
+
+ /**
+ * Reset JavaLogger.
+ */
+ @Override protected void afterTest() throws Exception {
+ GridTestUtils.setFieldValue(JavaLogger.class, JavaLogger.class,
"inited", false);
+ }
+
+
+ /**
+ * Check JavaLogger default constructor.
+ */
+ @Test
+ public void testDefaultConstructorWithDefaultConfig() {
+ IgniteLogger log1 = new JavaLogger();
+ IgniteLogger log2 = log1.getLogger(getClass());
+
+ assertTrue(log1.toString().contains("JavaLogger"));
+ assertTrue(log1.toString().contains(DFLT_CONFIG_PATH));
+
+ assertTrue(log2.toString().contains("JavaLogger"));
+ assertTrue(log2.toString().contains(DFLT_CONFIG_PATH));
+ }
+
+ /**
+ * Check JavaLogger constructor from java.util.logging.config.file
property.
+ */
+ @Test
+ public void testDefaultConstructorWithProperty() throws Exception {
+ File file = new File(U.getIgniteHome(), LOG_CONFIG_DEBUG);
+ System.setProperty("java.util.logging.config.file", file.getPath());
+ // Call readConfiguration explicitly because Logger.getLogger was
already called during IgniteUtils initialization
+ LogManager.getLogManager().readConfiguration();
+
+ IgniteLogger log1 = new JavaLogger();
+ assertTrue(log1.toString().contains("JavaLogger"));
+ assertTrue(log1.toString().contains(LOG_CONFIG_DEBUG));
+ assertTrue(log1.isDebugEnabled());
+
+ IgniteLogger log2 = log1.getLogger(getClass());
+ assertTrue(log2.toString().contains("JavaLogger"));
+ assertTrue(log2.toString().contains(LOG_CONFIG_DEBUG));
+ assertTrue(log1.isDebugEnabled());
+
+ System.clearProperty("java.util.logging.config.file");
+ }
+
+ /**
+ * Check Grid logging.
+ */
+ @Test
+ public void testGridLoggingWithDefaultLogger() throws Exception {
+ LogListener lsn = LogListener.matches("JavaLogger [quiet=true,")
+ .andMatches(DFLT_CONFIG_PATH)
+ .build();
+ ListeningTestLogger log = new ListeningTestLogger(new JavaLogger(),
lsn);
+
+ IgniteConfiguration cfg =
getConfiguration(getTestIgniteInstanceName());
+ cfg.setGridLogger(log);
+ startGrid(cfg);
+
+ assertTrue(GridTestUtils.waitForCondition(lsn::check, 2_000));
Review Comment:
Why do you wait for the check in background?
##########
modules/core/src/test/java/org/apache/ignite/testframework/junits/logger/GridTestLog4jLoggerSelfTest.java:
##########
@@ -64,28 +71,89 @@ public static void afterTests() {
assertEquals(defaultRootLevel,
LoggerContext.getContext(false).getConfiguration().getRootLogger().getLevel());
}
+ /** */
+ @Test
+ public void testFileConstructor() throws Exception {
+ File xml = GridTestUtils.resolveIgnitePath(LOG_PATH_TEST);
+
+ assert xml != null;
+ assert xml.exists();
+
+ IgniteLogger log = new GridTestLog4jLogger(xml).getLogger(getClass());
+
+ System.out.println(log.toString());
Review Comment:
do not sout anything in tests
##########
modules/core/src/test/java/org/apache/ignite/testframework/junits/logger/GridTestLog4jLogger.java:
##########
@@ -271,6 +271,23 @@ public GridTestLog4jLogger(final URL cfgUrl) throws
IgniteCheckedException {
quiet = quiet0;
}
+ /**
+ * Creates new logger with given implementation.
+ *
Review Comment:
Remove useless blank line
##########
modules/core/src/test/java/org/apache/ignite/testframework/junits/logger/GridTestLog4jLoggerSelfTest.java:
##########
@@ -64,28 +71,89 @@ public static void afterTests() {
assertEquals(defaultRootLevel,
LoggerContext.getContext(false).getConfiguration().getRootLogger().getLevel());
}
+ /** */
+ @Test
+ public void testFileConstructor() throws Exception {
+ File xml = GridTestUtils.resolveIgnitePath(LOG_PATH_TEST);
+
+ assert xml != null;
+ assert xml.exists();
Review Comment:
the existence is already tested in `GridTestUtils.resolveIgnitePath`
--
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...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
