Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate log4j-jul to JUnit 5 #3225

Draft
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

AlbaHerrerias
Copy link
Contributor

Hello 👋

We are from Neighbourhoodie, the implementation partner of the STF Bug Resilience Program. This work is part of our agreed Milestone 1. Upgrade from JUnit 4 to JUnit 5. This PR migrates the tests located in log4j-jul.

Thank you!

Checklist

  • Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
  • ./mvnw verify succeeds (if it fails due to code formatting issues reported by Spotless, simply run ./mvnw spotless:apply and retry)
  • Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
  • Tests for the changes are provided
  • Commits are signed (optional, but highly recommended)

@AlbaHerrerias
Copy link
Contributor Author

Hi, we are currently blocked and would like your advice. We've found out that the surefire-junit47 provider specified here only executes JUnit 4 tests. We've attempted to change the version and the provider with no luck. Is there another version of this dependency that would set up java.util.logging as required, but will also run JUnit 5 tests? Do you have any pointers for us? Thank you

@ppkarwasz
Copy link
Contributor

@AlbaHerrerias,

The problem with surefire-junit-platform was that either Surefire or JUnit 5 (I don't remember which one) initializes JUL (and therefore Log4j) way before control is transferred to the test class. Therefore this code executes too late:

public static void setUpClass() {
System.setProperty("java.util.logging.manager", LogManager.class.getName());
System.setProperty(Constants.LOGGER_ADAPTOR_PROPERTY, CoreLoggerAdapter.class.getName());
}

To workaround that, all the tests must be split into at least 4 Surefire runs, depending on the system properties that they need.
Most test classes require the java.util.logging.manager property to be set (except Log4jBridgeHandlerTest), but some need additional properties.
For example AsyncLoggerThreadsTest additionally needs the log4j2.contextSelector property to be set:

          <execution>
            <id>async-logger-test</id>
            <goals>
              <goal>test</goal>
            </goals>
            <phase>test</phase>
            <configuration>
              <includes>
                <include>**/AsyncLoggerThreadsTest.class</include>
              </includes>
              <!-- Use custom `j.u.l.LogManager` and an asynchronous selector -->
              <systemPropertyVariables>
                <java.util.logging.manager>org.apache.logging.jul.tolog4j.LogManager</java.util.logging.manager>
                <log4j2.contextSelector>org.apache.logging.log4j.core.async.AsyncLoggerContextSelector</log4j2.contextSelector>
              </systemPropertyVariables>
            </configuration>
          </execution>

CoreLoggerTest needs an additional log4j2.julLoggerAdapter system property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants