-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
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?
@@ -35,9 +35,9 @@ public void testCleanup() throws Exception { | |||
factory.getLogger("test"); | |||
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
.OverflowTest
is 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 refactorOverflowTest
to JUnit 5?Thank you!
Checklist
2.x
branch if you are targeting Log4j 2; usemain
otherwise./mvnw verify
succeeds (if it fails due to code formatting issues reported by Spotless, simply run./mvnw spotless:apply
and retry)src/changelog/.2.x.x
directory