IGNITE 19148 Switch tests from using JUL to log4j2 #2429

sk0x50 · 2023-08-09T10:36:12Z

https://issues.apache.org/jira/browse/IGNITE-19148

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Design: new code conforms with the design principles of the components it is added to.
Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
Code quality: code is clean and readable, necessary developer documentation is added if needed.
Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

Apache Ignite Coding Guidelines

…d log4j2)

Pochatkin · 2023-08-17T07:59:12Z

...re/src/testFixtures/java/org/apache/ignite/internal/testframework/log4j2/TestLogChecker.java

+
+        lock.writeLock().lock();
+        try {
+            for (Handler handler : handlers) {


Collections.addAll(this.handlers, handlers);

Thanks. Fixed

rpuch

What's the reason to change from JUL to log4j2? The JIRA only says about logging to stderr, but JUL can be configured to use any appenders (or how are they called there?) we want. So why?

rpuch · 2023-08-18T11:53:56Z

...i/src/integrationTest/java/org/apache/ignite/internal/cli/commands/ItClusterCommandTest.java


-        Handler physicalTopologyWaiter = physicalTopologyWaiter(allNodesAreInPhysicalTopology);
-        topologyLogger.addHandler(physicalTopologyWaiter);
+        TestLogChecker topologyLogger = new TestLogChecker(


This is not a logger, this is a checker. I suggest to rename this accordingly

You are absolutely right. Will fix it here and in other places.

rpuch · 2023-08-18T11:55:54Z

...re/src/testFixtures/java/org/apache/ignite/internal/testframework/log4j2/TestLogChecker.java

+import org.apache.logging.log4j.core.config.Property;
+
+/**
+ * Test log checker.


Let's expand the documentation with an explanation of what this checker does, what checks it performs, how it can be used

Javadoc was added.

rpuch · 2023-08-18T11:57:28Z

...re/src/testFixtures/java/org/apache/ignite/internal/testframework/log4j2/TestLogChecker.java

+     * @param loggerName Logger name.
+     * @param predicate Predicate to check log messages.
+     */
+    public TestLogChecker(String loggerName, Predicate<String> predicate) {


What happens if the predicate returns true or false?

Also, in some cases it is required to verify other message attributes, not only its text (for instance, we might want to verify that a message is logged as a WARN). Let's make the predicate accept the whole message, not just its text.

What happens if the predicate returns true or false?
updated javadoc in the following way:

/** * Creates a new instance of {@link TestLogChecker} with the given {@code predicate}. * In order to check that the required log event was logged, use {@link #isMatched()} method. * * @param loggerName Logger name. * @param predicate Predicate to check log messages. */

Also, in some cases it is required to verify other message attributes, not only its text
It makes sense to me. Predicate should accept LogEvent instead of String

rpuch · 2023-08-18T11:58:15Z

...re/src/testFixtures/java/org/apache/ignite/internal/testframework/log4j2/TestLogChecker.java

+    }
+
+    /**
+     * Creates a new instance of {@link TestLogChecker} with the given {@code predicate} and {@code action}.


There is no predicate among parameters, probably this needs to be adjusted

Thanks. Fixed.

rpuch · 2023-08-18T12:00:04Z

...re/src/testFixtures/java/org/apache/ignite/internal/testframework/log4j2/TestLogChecker.java

+    public Handler addHandler(Predicate<String> predicate, Runnable action) {
+        Handler handler = new Handler(predicate, action);
+
+        lock.writeLock().lock();


It seems we can just invoke addHandler(handler) here

Thanks. Fixed.

rpuch · 2023-08-18T12:08:01Z

...ntegrationTest/java/org/apache/ignite/network/scalecube/ItScaleCubeNetworkMessagingTest.java

    private final TestMessagesFactory messageFactory = new TestMessagesFactory();

+    /** List of test loggers. */
+    private final List<TestLogChecker> loggers = new ArrayList<>();


Those are not loggers, they are checkers

rpuch · 2023-08-18T12:09:28Z

...c/integrationTest/java/org/apache/ignite/internal/raftsnapshot/ItTableRaftSnapshotsTest.java

    private Cluster cluster;

-    private Logger replicatorLogger;
+    private TestLogChecker replicatorLogger;


This is not a logger anymore, the field probably needs to be renamed

rpuch · 2023-08-18T12:10:19Z

...c/integrationTest/java/org/apache/ignite/internal/raftsnapshot/ItTableRaftSnapshotsTest.java

        AtomicBoolean snapshotInstallFailedDueToIdenticalRetry = new AtomicBoolean(false);

-        Logger snapshotExecutorLogger = Logger.getLogger(SnapshotExecutorImpl.class.getName());
+        TestLogChecker snapshotExecutorLogger = TestLogChecker.create(SnapshotExecutorImpl.class);


This is not a logger

rpuch · 2023-08-18T12:54:46Z

...re/src/testFixtures/java/org/apache/ignite/internal/testframework/log4j2/TestLogChecker.java

+/**
+ * Test log checker.
+ */
+public class TestLogChecker {


The name does not seem perfect. 'Check' is too abstract; also this class not just 'checks' incoming log messages, but it also reacts to them somehow. Maybe a better name should be devised (but I cannot come up with anything good now)

Renamed to LogInspector

…order to allow to check log level etc

sk0x50

Hi @rpuch,

I agree that this is my fault and I had to describe the motivation in the ticket.

IMHO

It would be better to use a well-known and widely-used logging framework instead of JUL, which should not require any additional code like a custom appender, etc. I don't think that JUL is used in real-life solutions, so this patch can be considered a test scenario to use 3rd party implementation of the logging framework (For example, scalecube, which is an internal thing, uses slf4j and so we need to provide a bridge from slf4j to log4j2/logback. this step is not so obvious and has to be described in our documentation).
log4j2 provides a color schema for the logs, which is a nice feature :)

By the way, I don't like our approach of using logs to check something like cluster initialization, snapshots, etc. It should be done via special lifecycle events, callbacks, whatever... but using logs for this looks weird to me.

...re/src/testFixtures/java/org/apache/ignite/internal/testframework/log4j2/TestLogChecker.java

…testframework/log4j2/TestLogChecker.java Co-authored-by: Roman Puchkovskiy <roman.puchkovskiy@gmail.com>

sk0x50 force-pushed the ignite-19148 branch 2 times, most recently from 6d8d434 to 3a1c282 Compare August 14, 2023 11:34

sk0x50 added 17 commits August 16, 2023 16:42

IGNITE-19148 Switch tests implementation from using JUL to Log4j2

e0ba24e

IGNITE-19148 Add logger configuration

495d395

IGNITE-19148 Disable color schema

bdc5d15

IGNITE-19148 Revert pattern layout

38321ab

IGNITE-19148 Add a possible WA (introducing a bridge between slf4j an…

a7df6ae

…d log4j2)

IGNITE-19148 Fix ItScaleCubeNetworkMessaging test

5bd531f

IGNITE-19148 Fix ItClusterCommandTest

868f59d

IGNITE-19148 Fix ItTableRaftSnapshotsTest

dffb83d

IGNITE-19148 Fix ItStartTest

eaf8107

IGNITE-19148 Fix TestLogChecker

3e2a62d

IGNITE-19148 Remove NoOpHandler

232a24e

IGNITE-19148 Add colorization

6cd2117

IGNITE-19148 Add jansi dependency and simple cleanup

41610fa

IGNITE-19148 Return slf4j-jdk14 dependency for cli module

9e9cc78

IGNITE-19148 Remove java.util.logging.properties file

0bf47b8

IGNITE-19148 Remove unneeded dependencies

e7c4c60

IGNITE-19148 Disable colorization for team city

ead64ac

sk0x50 force-pushed the ignite-19148 branch from 3c449ba to ead64ac Compare August 16, 2023 13:46

IGNITE-19148 Remove debug code

b5a04c7

Pochatkin reviewed Aug 17, 2023

View reviewed changes

Pochatkin approved these changes Aug 17, 2023

View reviewed changes

sk0x50 added 2 commits August 17, 2023 12:44

IGNITE-19148 Using Collections.addAll instead of writing a loop directly

ccc3ebb

IGNITE-19148 Cosmetic changes

ad26ba2

rpuch reviewed Aug 18, 2023

View reviewed changes

sk0x50 added 3 commits August 21, 2023 10:19

IGNITE-19148 Rename loggers to checkers

26c4c8c

IGNITE-19148 Use Predicate<LogEvent> instaed of Predicate<String> in …

96304fa

…order to allow to check log level etc

IGNITE-19148 Add javadoc to TestLogChecker

f8bf164

sk0x50 added 2 commits August 21, 2023 11:50

IGNITE-19148 Corrected javadoc

5f74a19

IGNITE-19148 Fix compilation issue

8a56797

sk0x50 commented Aug 21, 2023

View reviewed changes

rpuch approved these changes Aug 21, 2023

View reviewed changes

...re/src/testFixtures/java/org/apache/ignite/internal/testframework/log4j2/TestLogChecker.java Outdated Show resolved Hide resolved

sk0x50 and others added 2 commits August 21, 2023 12:44

Update modules/core/src/testFixtures/java/org/apache/ignite/internal/…

39a9640

…testframework/log4j2/TestLogChecker.java Co-authored-by: Roman Puchkovskiy <roman.puchkovskiy@gmail.com>

IGNITE-19148 Rename TestLochChecker to LogInspector

b0b4ab3

sk0x50 merged commit e32fd8d into apache:main Aug 21, 2023

sk0x50 deleted the ignite-19148 branch September 1, 2023 11:47

IGNITE 19148 Switch tests from using JUL to log4j2 #2429

IGNITE 19148 Switch tests from using JUL to log4j2 #2429

Uh oh!

Conversation

sk0x50 commented Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The Review Checklist

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sk0x50 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sk0x50 commented Aug 9, 2023 •

edited

Loading