weavejester / eftest

Fast and pretty Clojure test runner
425 stars 40 forks source link

Capture test output and print it only for failing tests #24

Closed jarppe closed 7 years ago

jarppe commented 7 years ago

This PR captures *out*, *err*, System/out and System/err for each test thread into a ByteArrayOutputStream. The test reporter requests and prints the output for failing tests.

The implementation has a map eftest.output-capture/captured-output-contexts that is keyed by the thread id, and has the output capturing context as a value. The context has a buffer for output and a PrintWriter and PrintStream instances that both write to the buffer.

Before tests are run, the *out*, *err*, System/out and System/err are redirected to a proxied implementations of PrintWriter and PrintStream. These proxies intercept calls and redirect them to actual writer and stream found from captured-output-contexts selected by the current thread.

This allows keeping output from multiple threads separate, even with the System/out and System/err being shared by all threads.

Since the PrintStream and PrintWriter are concrete classes without suitable interfaces we can't use clojure.core/proxy. This implementation uses the javassist library for byte code generation.

The test reporter can then ask the generated output for the current thread and print it.

Deraen commented 7 years ago

At the least there should probably be an option to enable and disable this. Maybe :catch-output? option to run-tests?

weavejester commented 7 years ago

Thanks for the PR - I've added some review suggestions and questions.

jarppe commented 7 years ago

I reverted the version change and cleaned the set warning for reflection thing.

jarppe commented 7 years ago

Actually the PrintStream is not buffering the output to next OutputStream (thanks @miikka), so the solution you suggested seems to be working just fine.

I updated the PR so that the delegating proxy is created on OutputStream using clojure.lang/proxy and I dropped the dep to javassist.

Consequently the code is much shorter and simpler now.

I'm not sure how to test this, but I made a test project with lots of tests that were run in parallel, and I did not see any problems with the outputs.

weavejester commented 7 years ago

Looking at the source of PrintStream, it would appear that all methods write, then flush to the OutputStream within a synchronized block, which is why proxying the OutputStream is currently safe. While this may change in future (there doesn't seem to be a guarantee in the docs), it seems unlikely to, so it's probably something we can reasonably rely on.

weavejester commented 7 years ago

Oh, I should also add that I agree with @Deraen that there should be an option to disable this, as it might not be wanted or viable in some cases.

weavejester commented 7 years ago

This patch should be okay to merge, once an option is added to disable this functionality, and the commits are squashed and the commit message cleaned up.

jarppe commented 7 years ago

@weavejester Added option for output capture and commits squashed to one.

weavejester commented 7 years ago

Almost perfect. Could you make it so that :catch-out? being true is the default? I think, most of the time, catching the output is the right thing to do.

jarppe commented 7 years ago

The catch-out? is now true by default.

weavejester commented 7 years ago

Perfect, thanks!