Split JUnit 5 OutputCapture into extension and captured output

[philwebb]

The new OutputCapture class introduced in #14738 currently acts as both the extension and the captured output. This has the unfortunate side-effect of exposing JUnit callback methods to users of the API when really only JUnit cares about them. I think it's possible to split the class and offer an extension for JUnit to use as well as a user-facing class that represents the output captured by the extension.

[wilkinsona]

I have another OutputCapture extension locally in spring-boot-test-support as part of moving more of our tests to JUnit 5. We should align that new version with what happens here too.

[philwebb]

@wilkinsona I've just pushed the updated code. Let me know if you think any additional changes are needed.

[wilkinsona]

The javadoc for @RegisterExtension says it's typically used "in order to pass arguments to the extension's constructor, static factory method, or builder API". We're using it like this:

@RegisterExtension
CapturedOutput output = OutputExtension.capture();

I think it would be more idiomatic to use it declaratively at the class level:

@ExtendWith(OutputCaptureExtension.class)

Its ParameterResolver implementation would then allow injection of CapturedOutput into the test class or test method without a need for the capture() factory method. That's particularly nice on classes like EndpointIdTests where it's only used in one test as you can get rid of the field and just inject it as needed into the test method:

@Test
public void ofWhenContainsDeprecatedCharsLogsWarning(CapturedOutput capturedOutput) {
	EndpointId.resetLoggedWarnings();
	EndpointId.of("foo-bar");
	assertThat(capturedOutput).contains(
		"Endpoint ID 'foo-bar' contains invalid characters, please migrate to a valid format");
}

[wilkinsona]

Having looked more closely, the extension can already be used declaratively so the changes I tried to describe above become a matter of enforcing that. I think that's the right thing to do as it's more idiomatic. It also means that we can reduce the surface area of the API a little as the capture() factory method is no longer needed.

The first commit in this branch contains the changes that I think we should make. CapturedOutput no longer has any dependency on JUnit APIs. It could be moved to a separate package but this would require its push() and pop() methods to become public.

I explored splitting the capturing of the output and making that output available to tests into two separate classes in a general purpose org.springframework.boot.test.io package. This then allows both the JUnit 4 rule and the JUnit 5 extension to use the same underlying implementation. These changes are in the second commit of the same branch. It exposes the class that actually does the output capturing as public API, but we'd expect it to be only be used via the rule or extension. That's not ideal, but can't be avoided unless we repackage everything which would be a breaking change for the existing JUnit 4 rule.

[wilkinsona]

That's not ideal, but can't be avoided unless we repackage everything which would be a breaking change for the existing JUnit 4 rule.

I've pushed a third refinement that avoids this problem. It repackages things into an org.springframework.boot.test.io package and has both an extension and a rule that delegate to the same output capturing implementation. The existing rule has been updated to delegate to the new rule in the io package and has been deprecated.

[philwebb]

I like the the fact there's no dependency on the JUnit API's, but it's a bit annoying that you end up adding so many additional arguments to the tests. I guess you could always use constructor injection to setup a private instance.

The JUnit API is slightly annoying in that when you use @RegisterExtension the actual class must also be an Extension, otherwise it's silently dropped. It's a shame that the annotation alone isn't enough to trigger things. I'd rather the Extension check was done on the instance rather than the declared method type.

[sbrannen]

The JUnit API is slightly annoying in that when you use @RegisterExtension the actual class must also be an Extension, otherwise it's silently dropped. It's a shame that the annotation alone isn't enough to trigger things. I'd rather the Extension check was done on the instance rather than the declared method type.

Ask, and ye shall receive! junit-team/junit5#1908 😉

[sbrannen]

I have made additional proposals in #17049.

[wilkinsona]

Re-opening again as SystemCapture isn't thread-safe. This can result in a concurrent modification exception if something writes to System.out or System.error while append is iterating over the list of captured strings.

[wilkinsona]

The thread-safety problem was fixed as part of b18fffa.