-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Migrate log4j-slf4j2-impl to JUnit 5
#3080
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-slf4j2-impl to JUnit 5
#3080
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks!
Since the log4j-slf4j-impl module is mostly identical to this one (it has a couple of tests less). Could you apply the changes to that module too?
Could you also remove the junit-vintage-engine dependency in the pom.xml to show that not tests are using JUnit 4?
| Set<LoggerContext> set = factory.getLoggerContexts(); | ||
| final LoggerContext ctx1 = set.toArray(LoggerContext.EMPTY_ARRAY)[0]; | ||
| assertTrue("LoggerContext is not enabled for shutdown", ctx1 instanceof LifeCycle); | ||
| assertTrue(ctx1 instanceof LifeCycle, "LoggerContext is not enabled for shutdown"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assertTrue(ctx1 instanceof LifeCycle, "LoggerContext is not enabled for shutdown"); | |
| assertInstanceOf(LifeCycle.class, ctx1, "LoggerContext is not enabled for shutdown"); |
Could you change the assertions on X instanceof Y to use the new assertInstanceOf assertions?
As far as I can tell <plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<executions>
<!-- Separate test execution to verify that the presence of both:
~ * `log4j-slf4j2-impl` (bridge from SLF4J to Log4j API)
~ * `log4j-to-slf4j` (bridge from Log4j API to SLF4J)
~ does not cause a stack overflow.
-->
<execution>
<id>loop-test</id>
<goals>
<goal>test</goal>
</goals>
<configuration>
<additionalClasspathDependencies>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-to-slf4j</artifactId>
<version>${project.version}</version>
</dependency>
</additionalClasspathDependencies>
<includes>
<include>**/OverflowTest.java</include>
</includes>
</configuration>
</execution>
<execution>
<id>default-test</id>
<configuration>
<includes>
<include>**/*Test.java</include>
</includes>
<excludes>
<exclude>**/OverflowTest.java</exclude>
</excludes>
</configuration>
</execution>
</executions>
</plugin>and |
c0b8144 to
4c23edd
Compare
|
Thank you for the review and assistance, I have addressed now your requested changes. Please let me know if you'd like to request more. I will make the refactor of the Also note that, with the removal of |
4c23edd to
67a954c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks! 🚀
No need to port this to main, since the 3.x artifact will be removed soon (#2924).
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-slf4j2-impl.OverflowTestis set to run specifically with Junit 4. This behaviour was added not long ago so we wanted to ask, should we leave it like this or should we refactorOverflowTestto JUnit 5?Thank you!
Checklist
2.xbranch if you are targeting Log4j 2; usemainotherwise./mvnw verifysucceeds (if it fails due to code formatting issues reported by Spotless, simply run./mvnw spotless:applyand retry)src/changelog/.2.x.xdirectory