-
Notifications
You must be signed in to change notification settings - Fork 185
Stop swallowing System.err in JUnit tests #2685
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
Stop swallowing System.err in JUnit tests #2685
Conversation
While working on eclipse-platform#2685 and specifically trying to verify that change fixes the comment below \* I realized that a migration step was missed changing the suites from JUnit4 to JUnit5. On JUnit5 the `@BeforeAll` and `@AfterAll` only apply to the tests in the class itself, not to `@SelectClasses` too. Instead JUnit5 provides `@BeforeSuite` and `@AfterSuite` \* this comment: > For some reason, printing to System.err in JUnit test has no effect
test_printStackTrace overrides System.err with its own print stream to check functionality. But these tests did not restore System.err, so all System.err output for the rest of the test suite was lost into the last of these ByteArrayOutputStreams that was created. The tests themselves have little intrinsic value as they really test nothing of SWT itself, but it does help achieve code coverage. This was discovered when working on eclipse-platform#2684 as I could not see the output when running all the tests. Since we no longer swallow errors, the "For some reason, printing to System.err[...]" comment is resolved and we can print leaks to stderr again.
Test Results 111 files - 7 111 suites - 7 14m 19s ⏱️ +19s For more details on these failures, see this check. Results for commit 785c322. ± Comparison against base commit c308fd1. This pull request removes 56 tests.♻️ This comment has been updated with latest results. |
c8dfa71 to
785c322
Compare
While working on eclipse-platform#2685 and specifically trying to verify that change fixes the comment below \* I realized that a migration step was missed changing the suites from JUnit4 to JUnit5. On JUnit5 the `@BeforeAll` and `@AfterAll` only apply to the tests in the class itself, not to `@SelectClasses` too. Instead JUnit5 provides `@BeforeSuite` and `@AfterSuite` \* this comment: > For some reason, printing to System.err in JUnit test has no effect
| for (Error leak : leakedResources) { | ||
| // For some reason, printing to System.err in JUnit test has no effect | ||
| leak.printStackTrace (System.out); | ||
| leak.printStackTrace (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @SyntevoAlex I found out where System.err was going.
While working on #2685 and specifically trying to verify that change fixes the comment below \* I realized that a migration step was missed changing the suites from JUnit4 to JUnit5. On JUnit5 the `@BeforeAll` and `@AfterAll` only apply to the tests in the class itself, not to `@SelectClasses` too. Instead JUnit5 provides `@BeforeSuite` and `@AfterSuite` \* this comment: > For some reason, printing to System.err in JUnit test has no effect
| * @param runnable to run while capturing output | ||
| * @return output on System.err | ||
| */ | ||
| public static String runWithCapturedStderr(Runnable runnable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I've recently relied on AutoCloseable a lot for testing; it allows some good way of wrapping state relying less on Runnable.
For this case, it could go that way
class CaptureStderr implements AutoCloseable, Supplier<String> {
ByteArrayOutputStream errContent
CaptureStderr() {
errContent = new ByteArrayOutputStream();
System.setErr(new PrintStream(errContent, true, StandardCharsets.UTF_8));
}
@Override
public void close() {
System.setErr(stderr);
errContent.close();
}
@Override
public String get() {
return errContent.toString(StandardCharsets.UTF_8);
}
}and then tests can be written
try (new captureStdErr = new CaptureStdErr()) {
// do stuff
assertTrue(captureStdErr.get().contains("blah");
}
test_printStackTrace overrides System.err with its own print stream to check functionality. But these tests did not restore System.err, so all System.err output for the rest of the test suite was lost into the last of these ByteArrayOutputStreams that was created.
The tests themselves have little intrinsic value as they really test nothing of SWT itself, but it does help achieve code coverage.
This was discovered when working on #2684 as I could not see the output when running all the tests.