[SUREFIRE-1934] Ability to disable system-out/system-err for successfully passed tests

[NissMoony]

Added ability to disable system-out and system-err blocks through the plugin configuration.
For some tests the output too big and our pipeline tool cannot parse it.
Added it-tests.

[michael-o]

Quite reasonable, one nit though.

[michael-o]

Also, were can I see that this applies to passed tests only?

[michael-o]

@NissMoony Please let me know whether you want to be merged.

[kriegaex]

Also, were can I see that this applies to passed tests only?

Yes, it seems that the TestOutElements classes need one negative test case each, too.

@NissMoony, I guess all suggested changes are rather trivial to implement. If you give this PR some attention now, @michael-o, who is currently working on a bunch of at least two other output-stream-related PRs, could move forward more quickly and consider them all together as a whole.

@michael-o, I am not a committer, but I guess you can just commit those trivial changes on top of the PR branch yourself, if you think the PR is valuable and otherwise OK. I have not reviewed it, just found it by chance.

[kriegaex]

Without having seen the code, the basic idea sounds appealing. I lately had discussions in another project, because there were some tests with a lot of output, including some (expected) logged exception stack traces, which annoyed the reviewers, while I thought it was important to see the output in case of a test failure. So, to retain the output in case of an error and to suppress them if the tests pass, it actually a good idea. It can be dangerous too, if tests pass for the wrong reasons (e.g. weak assertions) without anyone noticing, but that is another topic.

Over in the other project, I was urged to mock stdOut and stdErr to silence the tests, but that is not thread-safe, so I had to mark the tests as @Isolated, which I hate to do. So for reviewing this PR, I think it is essential to make sure that thread-safety is handled correctly. After all, there is one one static instance of System.out and System.err each. If either is suppressed for test A and test B runs in parallel, one is failing and the other passing, this could be a problem. Have you checked that? I am just a passer-by here, and my review does not carry much weight anyway, not being a committer. I am just mentioning it. Maybe @NissMoony can explain whether - and if so, how - he (she?) took care of the problem.

Update: OK, I quickly looked at the code. This PR does nothing of the sort I was talking about above. It does not change stdOut/stdErr capturing in any way, just adds a switch adding the output to the XML report. I.e., which ever way Surefire handles those streams in a multi-threading situation, remains the same. Sorry for the misunderstanding and for the noise. I had hoped that someone came up with a magical solution for the root problem. But it is not being addressed here.

[NissMoony]

Please let me know whether you want to be merged.

Yes, the problem really important fo our project. Sorry for long response, not much time in the end of year. I'll fix remarks, but now I can see one problem.

Also, were can I see that this applies to passed tests only?

I'm little misunderstood issue that I linked with. For few our projects there is important to disable all output elements for all success and failure tests (how it worked in previous versions).
@michael-o, what behaviour do you think is required as part of the plugin: the ability to disable output for all tests (as in this PR, my cases) or for successful tests only (as initially asked in issue)?

[kriegaex]

what behaviour do you think is required as part of the plugin: the ability to disable output for all tests (as in this PR, my cases) or for successful tests only (as initially asked in issue)?

I am not Michael, but IMO it makes sense to have both. The simplest way to do that would be two boolean options, one for failed tests and tests in error and one for passing tests. An alternative would be some kind of list with a set of fixed values, but then you also need to add all possible combinations to the list and name them accordingly. With two options, this is not much, but what if in the future we want to support more test statuses, e.g. differentiate failed tests from tests in error, or what if skipped tests in some tools can print the reason for skipping to the console? You get the picture.

I am sure, @michael-o can guide you towards a good design decision.

[michael-o]

Please let me know whether you want to be merged.

Yes, the problem really important fo our project. Sorry for long response, not much time in the end of year. I'll fix remarks, but now I can see one problem.

Also, were can I see that this applies to passed tests only?

I'm little misunderstood issue that I linked with. For few our projects there is important to disable all output elements for all success and failure tests (how it worked in previous versions). @michael-o, what behaviour do you think is required as part of the plugin: the ability to disable output for all tests (as in this PR, my cases) or for successful tests only (as initially asked in issue)?

Scratch my previous statement. Re-reviewed your PR, from a static PoV, it looks reasonable. In fact, I picked up your idea and created #702. After it is merged, I will go over to your PR.

[michael-o]

I have now re-read the code and your change. I fail to see how this change only applies to successful tests. It applies to all type of results: SUCCESS and non-SUCCESS otherwise you wouldn't modify #getTestProblems(), would you?
This is not necessarily wrong, but inconsistent with your description. See the original change: b803256#diff-c634b539151b4b76bde50d93d5a6cd2b0f51aa411281398cc295cc721bdfa1d5

[zabetak]

Hey @michael-o, according to a previous comment it seems that the intention of @NissMoony was to provide an option for suppressing all output (irrespective of the type of the result) from the XML report.

For our use-case (Apache Hive), I think that disabling all output from the XML report is sufficient to move our upgrade further assuming that redirectTestOutputToFile option works as before and will still send the STDOUT to a file irrespective of the the changes introduced by this PR.

Having more fine grained configurations (e.g., only apply this on successful/failed tests) may be a nice to have feature but not a strong requirement from our side at this point. We all understand that complex configurations are hard to maintain.

[michael-o]

Hey @michael-o, according to a previous comment it seems that the intention of @NissMoony was to provide an option for suppressing all output (irrespective of the type of the result) from the XML report.

For our use-case (Apache Hive), I think that disabling all output from the XML report is sufficient to move our upgrade further assuming that redirectTestOutputToFile option works as before and will still send the STDOUT to a file irrespective of the the changes introduced by this PR.

Having more fine grained configurations (e.g., only apply this on successful/failed tests) may be a nice to have feature but not a strong requirement from our side at this point. We all understand that complex configurations are hard to maintain.

The code contradicts the issue summary. Either one should be picked.

[zabetak]

The code contradicts the issue summary. Either one should be picked.

Sure that is an obvious problem. It seems though that we all kind of agree with the idea in the code so we could just edit the summary and move this forward.

[michael-o]

The code contradicts the issue summary. Either one should be picked.

Sure that is an obvious problem. It seems though that we all kind of agree with the idea in the code so we could just edit the summary and move this forward.

I believe that it was wrong from begin with to make this non-optional. Means: Output for successful tests should be disabled by default and not the other way around.

[zabetak]

Personally, most of the time I would prefer not to have the STDOUT/STDERR in the XML report especially for tests that run successfully. I think the best would be to configure loggers accordingly and don't have to play with the surefire configuration but sometimes this is inevitable.

[turbanoff]

I believe most of developers want to have logs for failed tests. Because often it's what we only have to investigate test failures on CI. Sometimes for rarely failing tests it's the only way to diagnose the problems.

[michael-o]

Guys, I have rebased and reworked the PR a bit. Please have a look.

[michael-o]

For me, this is complete now. Please have a look.

[michael-o]

Has improved tests now