[SUREFIRE-1659] Log4j logger in TestExecutionListener corrupts Surefire's STDOUT #342
Conversation
kalgon
changed the title
| import org.junit.platform.launcher.TestPlan; | ||
| import org.junit.platform.launcher.core.LauncherFactory; | ||
|
|
||
| class LazyLauncher implements Launcher |
There was a problem hiding this comment.
Pls put a Javadoc for the class with the purpose of the class.
|
@kalgon Can you extend the integration test |
|
@Tibor17 Thanks for the feedback and for pointing me how to test the fix (I really had no idea how to test that). I'll take a look at it. |
| @@ -49,12 +49,42 @@ public void warningIsNotEmitted() throws VerificationException | |||
| { | |||
| OutputValidator validator = unpack( "/surefire-1614-stream-corruption" ) | |||
| .executeTest() | |||
| .verifyErrorFree( 1 ); | |||
| .verifyErrorFree( 2 ); | |||
There was a problem hiding this comment.
Did you change the project within surefire-1614-stream-corruption? I do not understand why 2 tests.
There was a problem hiding this comment.
You're right, this line should not have changed. I first tried to add a new test in surefire-1614-stream-corruption (that's where the 2 comes from) but in the end, I created a new mini-project in surefire-1659-stream-corruption because the easiest way to trigger the problem is to have a junit-platform.properties and a redirection from java.util.logging to any other logging framework (I tested with both log4j and slf4j/logback). I'll change that 2 back to 1 ASAP. Do you have any other remark?
There was a problem hiding this comment.
Thx for making it clear. Yes there are some little changes only.
|
|
||
| <properties> | ||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <maven.compiler.source>1.8</maven.compiler.source> |
There was a problem hiding this comment.
Please turn 1.8 to ${java.specification.version}.
| <dependency> | ||
| <groupId>org.junit.jupiter</groupId> | ||
| <artifactId>junit-jupiter-engine</artifactId> | ||
| <version>5.7.0</version> |
There was a problem hiding this comment.
Is 5.7.0 mandatory?
Can we use 5.7.1?
|
Finally, squash the commits and rename the final commit to the name of PR. |
Surefire's STDOUT
|
@kalgon |
Fix candidate for SUREFIRE-1659