Skip to content
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

Polish OutputCapture and its JUnit Jupiter extension #17049

Closed

Conversation

sbrannen
Copy link
Member

@sbrannen sbrannen commented Jun 3, 2019

This PR contains a number of individual commits to improve the output capture extensions for JUnit 4 and JUnit Jupiter.

Commit 490e315 is not essential, but the changes will likely be necessary to support parallel test execution. The subsequent commit (8ce1237) introduces a failing, disabled test case which demonstrates that parallel execution is currently not supported. If the team decides not to support parallel test execution, I recommend that a note be added to the Javadoc and Reference Manual to document this known shortcoming.

sbrannen added 4 commits June 3, 2019 13:57
This commit refactors OutputCaptureExtension so that state is stored in
the ExtensionContext.Store instead of in the extension.

This is more idiomatic for JUnit Jupiter extensions and allows for
potential concurrent use of the extension.
This commit introduces failing tests for the OutputCaptureExtension,
which are currently @disabled.
@wilkinsona
Copy link
Member

Thanks, @sbrannen. This is quite timely as I had just come to the realisation that storing the CapturedOutput instance might be useful for similar reasons to those discussed in the JUnit issue about making @TempDir work in an extension. I'm not sure it'll work, but the changes here will give me an easy way to try it out at least.

@wilkinsona wilkinsona self-assigned this Jun 3, 2019
@sbrannen
Copy link
Member Author

sbrannen commented Jun 3, 2019

Sure. No problem.

FWIW, I already did a spike for parallel execution support by storing capturedStrings in SystemCapture in a ThreadLocal. That gets you pretty far but not all the way. The issue is that different threads write to System.err and System.out. Of course, we expect that to be the "challenge".

Perhaps you have an idea of how to solve that. If not, you might consider the implementation in the JUnit Platform for inspiration.

@wilkinsona
Copy link
Member

It might be possible to support output capture during parallel test execution using InheritableThreadLocal but I'm not sure that it will be worth the effort. Asserting captured output is already something of a last resort so I don't think it would be too much of a problem to only support it for serial test execution.

@philwebb
Copy link
Member

philwebb commented Jun 3, 2019

I'm not sure we should support parallel execution for now. We probably ought to just wait until we hit the need for it ourselves.

@sbrannen
Copy link
Member Author

sbrannen commented Jun 4, 2019

I'm not sure we should support parallel execution for now.

I can certainly understand that. In my opinion, it's only a "nice to have" feature.

So, if the community doesn't really request/need it, then postponing any work on it is justified.

I still think it would be worth it to document that fact in the Javadoc and Boot reference docs.

@wilkinsona wilkinsona changed the title Improve output capture extensions Polish OutputCapture extensions Jul 18, 2019
@wilkinsona wilkinsona added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 18, 2019
@wilkinsona wilkinsona added this to the 2.2.x milestone Jul 18, 2019
@wilkinsona wilkinsona changed the title Polish OutputCapture extensions Polish OutputCapture and its JUnit Jupiter extension Jul 18, 2019
wilkinsona pushed a commit that referenced this pull request Jul 18, 2019
- Polish Javadoc
- Improve error message in OutputCapture
- Use ExtensionContext.Store in OutputCaptureExtension

See gh-17049
@wilkinsona wilkinsona modified the milestones: 2.2.x, 2.2.0.M5 Jul 18, 2019
@wilkinsona
Copy link
Member

Thanks very much, @sbrannen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants