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

Merged
merged 1 commit into from
Dec 10, 2024

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.

@AlbaHerrerias AlbaHerrerias force-pushed the log4j-jul-migration-junit5 branch from 552b858 to 1fe1c2d Compare December 10, 2024 15:52
@AlbaHerrerias AlbaHerrerias force-pushed the log4j-jul-migration-junit5 branch from 1fe1c2d to 7ecae66 Compare December 10, 2024 16:08
@AlbaHerrerias AlbaHerrerias marked this pull request as ready for review December 10, 2024 16:09
@AlbaHerrerias
Copy link
Contributor Author

Hello @ppkarwasz , thank you for your help, it unblocked me.

Copy link

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-macos-latest clean install 3.9.8 Build Scan PUBLISHED
build-ubuntu-latest clean install 3.9.8 Build Scan PUBLISHED
build-windows-latest clean install 3.9.8 Build Scan PUBLISHED
Generated by gradle/develocity-actions

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlbaHerrerias,

LGTM, thanks!

@ppkarwasz ppkarwasz merged commit 0f48f60 into apache:2.x Dec 10, 2024
9 checks passed
@AlbaHerrerias
Copy link
Contributor Author

Hello @ppkarwasz. I wanted to ask if this refactor needs to be ported to main. Previously, your team has been handling the porting, and I was wondering if you plan to continue doing so for this and the other modules we've refactored that are not yet in main. Thank you! 🙏

@ppkarwasz
Copy link
Contributor

Previously, your team has been handling the porting, and I was wondering if you plan to continue doing so for this and the other modules we've refactored that are not yet in main. Thank you! 🙏

I have the impression that you did all the ports to main (of course for modules that still exist in main). The version of log4j-jul in main is strongly amputated, so no need to migrate this one.

@AlbaHerrerias
Copy link
Contributor Author

AlbaHerrerias commented Dec 12, 2024

@ppkarwasz
They were made by @vy 🙇

I've compiled a small list with our changes:

PR against 2.x Ported to main
log4j-jdbc-dbcp2 Yes, done by vy
log4j-jpl Yes, done by vy
log4j-to-slf4j No need
log4j-slf4j-impl No need
log4j-slf4j2-impl No need
log4j-jul No need
log4j-jpa No need
log4j-jakarta-smtp Does not look it is in main
log4j-taglib Does not look it is in main
log4j-1.2-api Does not look it is in main
log4j-api-test (not finished, we need assistance) Does not look it is in main
log4j-osgi-test
log4j-core-test (not merged)
log4j-iostreams (not merged)

Am I correct in understanding that three modules require porting? (log4j-osgi-test, log4j-core-test, log4j-iostreams). Would you prefer to handle the porting directly, or would you like us to open the PRs against main?

Please note this PR #3221 was related to the migration but not done by us. If I'm not mistaken this is not in main either.

Thank you

@ppkarwasz
Copy link
Contributor

Am I correct in understanding that three modules require porting? (log4j-osgi-test, log4j-core-test, log4j-iostreams). Would you prefer to handle the porting directly, or would you like us to open the PRs against main?

There might be a lot of differences in log4j-core-test between 2.x and main, so please make a separate PR for main if you can. log4j-iostreams shouldn't event be in main and I can port log4j-osgi-test myself.

Please note this PR #3221 was related to the migration but not done by us. If I'm not mistaken this is not in main either.

Thanks for the remark, we didn't notice that, sorry. We'll double-check that PR to see if it contains independent work or is just a squash of all your PRs.

@AlbaHerrerias
Copy link
Contributor Author

There might be a lot of differences in log4j-core-test between 2.x and main, so please make a separate PR for main if you can. log4j-iostreams shouldn't event be in main and I can port log4j-osgi-test myself.

Thank you. I've incorporated the applicable changes from log4j-core-test into main in this PR.

Thanks for the remark, we didn't notice that, sorry. We'll double-check that PR to see if it contains independent work or is just a squash of all your PRs.

It contains independent work, I don't think it include our changes

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