Surefire 1614 #209

sormuras · 2018-12-12T22:07:45Z

Capture system stream earlier to prevent its corruption.

eolivelli

Looks good.
Cose is cleaner now

+1

Tibor17 · 2018-12-12T22:32:29Z

...it-platform/src/main/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProvider.java

        }
        else
        {
            throw new IllegalArgumentException( "Unexpected value of forkTestSet: " + forkTestSet );
        }
+
+        return reporterFactory.close();


Why you do not call it in finally block as before?

Wouldn't it swallow the IllegalArgumentException?

It's not about exceptions. The factory has to be finally closed. The code in close must be called always because the code make a report.

@sormuras
Don't break the meaning and behavior of previous code. The close is not your problem. The problem was with method scanClasspath and not the close method.

Tibor17 · 2018-12-12T22:35:37Z

...it-platform/src/main/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProvider.java

@@ -104,22 +104,29 @@ public JUnitPlatformProvider( ProviderParameters parameters )
    public RunResult invoke( Object forkTestSet )
                    throws TestSetFailedException, ReporterException
    {
+        ReporterFactory reporterFactory = parameters.getReporterFactory();


All this PR has the same call sequences as before.
I still do not see logical sense for this change because there is no change in call sequences.
What root cause you found?

@sormuras
I found the root cause. It was caused by calling method scanClasspath() before startCapture.
Change your commit and use try-finally in the method invoke and use the code within finally block as before.

Good catch!

Know I understand the reason for that strange try/finally. Can we add a comment?

Tibor17 · 2018-12-12T22:42:08Z

...t/resources/surefire-1614-stream-corruption/src/test/java/com/example/demo/CustomRunner.java

+import org.junit.runners.model.InitializationError;
+import org.junit.runners.model.TestClass;
+
+public class CustomRunner extends BlockJUnit4ClassRunner {


Still not the ASF Maven style.
Why if you type it correctly in src/main/java, this is another check style.

So we are not running checkstyle on IT cases? Fair. But we can consider adding it.
For the future I mean

I just copied the sample project files. Did not manually type a character here.

So we are not running checkstyle on IT cases? Fair. But we can consider adding it.

That'd be wonderful! As long as "mvn verify" exits with "0", I'm fine with the format.

Even better: have profile or task that applies the desired formatting automatically via spotless or other formatting tool. For all sources, be it src/main, src/test, or src/it.

If you use IDEA14 ad it is configured properly, just press CTRL+ALT+L and the files are adjusted to ASF style check.

Well (...) never mind.

@sormuras
You have to follow the code style and I guess you want to work in ASF. It's easy!

All I'm saying is that when an almost 2 hour running build doesn't fail due to code formatting issues, everything's fine for me too.

[...] use IDEA14 ad it is configured properly [...]

There are tools that a) format and b) check for style violations. We should use them -- and not rely on personal IDEs, their proper configuration and hope that stars align. If such tools were used you even gain speed when reviewing PRs: a successful build means that all style checks passed. Yeah!

You have to follow the code style

Sure. Still I expect from properly setup projects to notify me about code style violations. A build should not even start to compile stuff when there are code style violations present.

I guess you want to work in ASF

?

@sormuras
Again we have a written code style as rules in ASF Maven. This is still not clear? I am only saying that tools help you to adjust it faster. Otherwise you can do it by your hands but it will be more painful for you. Is it clear now?

Tibor17 · 2018-12-13T19:26:06Z

@sormuras
Pls make me committer in your PR and I will finish the code style and squash it. Thx.

sormuras · 2018-12-14T03:14:43Z

You're welcome. Just work on this branch: https://github.com/apache/maven-surefire/tree/SUREFIRE-1614

…ire's STDOUT when using JUnit's Vintage Engine

marcphilipp · 2018-12-14T22:51:33Z

@Tibor17 What version is this going to be released in?

Tibor17 · 2018-12-15T06:32:45Z

@marcphilipp The version is 3.0.0-M3. It does not make sense to wait for final version. The version is stable enough for JUnit5 project and the issue like this which print an annoying Warning do not break the build because it's a warning. We of course missed out the features display name and logs isolation in parallel execution because we do not have resources to complete it. We have got one more committer these days, so I hope that he helps us a bit.

marcphilipp · 2018-12-15T10:17:46Z

Well, projects like Spring Boot are waiting for an official release that includes this fix and they won't recommend using a milestone version to their users. Would it be possible to backport the fix to a 2.x release?

Tibor17 · 2018-12-15T11:56:54Z

@marcphilipp
This was explained few times already.
These release candidates are stable and there is no risk to use them by Spring.
There should not be fear that it is 50% of functionality. The plugins contain everything as before.
It is a set of three plugins which are related to Maven API 3.0 which was completed in M1 and the next work was split in several release candidates.
The version 3.0 is a big chance to make such a change which cannot be done in maintenance of 2.x.
The roadmap is on the frontpage and the final release will remove deprecated configuration parameters but I do not think that it is a problem for users to change POM. Meanwhile we will change internal API and impl and since some teams rely on the API it cannot be done after 3.0. I hope this explained all.

Tibor17 · 2018-12-15T11:57:41Z

@marcphilipp
See the roadmap https://maven.apache.org/surefire/maven-surefire-plugin/

marcphilipp · 2018-12-15T12:51:06Z

Thanks for sharing the link to the roadmap! Since an M typically stands for "milestone" and not "release candidate", it wasn't clear to me that you consider these ready for production use. If they are indeed stable, why not make them official releases?

I know from experience that it's hard to commit to dates when you mostly work on a project in your free time but is there a timeline for 3.0?

I'm asking because the issue fixed by this PR is one of the main blockers for Spring Boot's adoption of JUnit 5 (see spring-projects/spring-boot#14736 (comment)).

Tibor17 · 2018-12-15T18:53:17Z

@marcphilipp
I told you again why. If I could I would already do it.

the issue fixed by this PR is one of the main blockers for Spring Boot's

yes, and there will be 3.0.0-M3 for this PR. But this is not a bug in this PR. It is only a removal of warning which does not affect behavior. If somebody is talking about corrupted stream, it is usually caused by e.g. Karaf and Arquillian which use or used STDOUT and STDIN streams and broke Surefire's communication channel. Our plan is to break the compatibility and change the way how two JVMs communicate. Therefore it can be done on the change from last version 2.x (2.22.1) to 3.x. But because there is so much work like this change and more, one release candidate is not enough to cut.
For normal users it represents no change of course. For those who maybe have their own Providers it would be a disaster to change something like that between 3.0 and 3.1 let's say. Therefore everything has to be done in 3.0 and these milestones of work from the backlog.

eolivelli approved these changes Dec 12, 2018

View reviewed changes

Tibor17 reviewed Dec 12, 2018

View reviewed changes

[SUREFIRE-1614] JUnit Runner that writes to System.out corrupts Suref…

f517d34

…ire's STDOUT when using JUnit's Vintage Engine

asfgit force-pushed the SUREFIRE-1614 branch from 938d422 to f517d34 Compare December 14, 2018 20:38

asfgit merged commit f517d34 into master Dec 14, 2018

asfgit deleted the SUREFIRE-1614 branch December 14, 2018 22:43

dupski mentioned this pull request Feb 14, 2019

Upgrade spring boot starter test to JUnit 5 spring-projects/spring-boot#14736

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Surefire 1614 #209

Surefire 1614 #209

sormuras commented Dec 12, 2018

eolivelli left a comment

Tibor17 Dec 12, 2018

sormuras Dec 13, 2018

Tibor17 Dec 13, 2018

Tibor17 Dec 13, 2018

Tibor17 Dec 12, 2018

Tibor17 Dec 12, 2018

eolivelli Dec 13, 2018

Tibor17 Dec 12, 2018 •

edited

Loading

eolivelli Dec 13, 2018 •

edited

Loading

sormuras Dec 13, 2018 •

edited

Loading

Tibor17 Dec 13, 2018

sormuras Dec 13, 2018

Tibor17 Dec 13, 2018

sormuras Dec 13, 2018

Tibor17 Dec 13, 2018

Tibor17 commented Dec 13, 2018

sormuras commented Dec 14, 2018

marcphilipp commented Dec 14, 2018

Tibor17 commented Dec 15, 2018 •

edited

Loading

marcphilipp commented Dec 15, 2018

Tibor17 commented Dec 15, 2018 •

edited

Loading

Tibor17 commented Dec 15, 2018

marcphilipp commented Dec 15, 2018

Tibor17 commented Dec 15, 2018 •

edited

Loading

Surefire 1614 #209

Surefire 1614 #209

Conversation

sormuras commented Dec 12, 2018

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

eolivelli Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

sormuras Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 commented Dec 13, 2018

sormuras commented Dec 14, 2018

marcphilipp commented Dec 14, 2018

Tibor17 commented Dec 15, 2018 • edited Loading

marcphilipp commented Dec 15, 2018

Tibor17 commented Dec 15, 2018 • edited Loading

Tibor17 commented Dec 15, 2018

marcphilipp commented Dec 15, 2018

Tibor17 commented Dec 15, 2018 • edited Loading

Tibor17 Dec 12, 2018 •

edited

Loading

eolivelli Dec 13, 2018 •

edited

Loading

sormuras Dec 13, 2018 •

edited

Loading

Tibor17 commented Dec 15, 2018 •

edited

Loading

Tibor17 commented Dec 15, 2018 •

edited

Loading

Tibor17 commented Dec 15, 2018 •

edited

Loading