Support for JUnit test output

[miikka]

There's already clojure.test.junit, which would give me JUnit output, except:

• it's not thread-safe, so the output will be screwed up if used as Eftest's reporting function
• I'd like to have JUnit XML files in addition to the familiar human-readable output.

This patch implements a more thread-safe wrapper for clojure.test.junit and adds an Eftest option for using it to write to a file.

---

Sorry for sending code your way without opening an issue to discuss this first. I thought I'd take a quick stab at it to identify implementation problems before opening an issue, but well, I didn't run into any problems. Admittedly using push-thread-bindings and pop-thread-bindings is a bit awkward. If you do not want to have it or want significant changes, do not hesitate to let me know.

[weavejester]

Thanks for the patch! I think there's an easier way to do this, though.

First of all, there already exists a clojure.test.junit namespace. It has a reporter that we can probably reuse, and we can use the :begin-test-run and :summary events to setup the beginning and end of the XML.

Secondly, we don't need to treat junit as a separate case. We can have multiple reporting functions just by running them in sequence. For example:

(run-tests
 (find-tests "test")
 {:report (fn [m]
            (eftest.report.pretty/report m)
            (eftest.report.junit/report m)})

We might need some fixes around using eftest.report/*context* in a way that doesn't affect other reporters (namespaced keywords, using swap! over reset! etc.), but that's a separate issue, and shouldn't affect the junit reporter, I think.

[weavejester]

Oh, I see you're already using clojure.test.junit! Why are you using push-thread-bindings in that case?

[weavejester]

Ah, okay, now that I look more closely at both namespaces I believe I see what you're doing, at least in part.

clojure.test.junit makes use of dynamic bindings, and you're using push-thread-bindings to simulate a binding call. I still don't understand your use of *test-result*, however.

I'm beginning to think that requiring clojure.test.junit is actually more a hinderance than an advantage. Eftest and Clojure use the same license, so we can just rip what we need from the clojure.test.junit namespace and adapt it for eftest.report.junit.

In place of *depth* and *var-context* we instead use the eftest.report/*context* map. In place of with-junit-output we use the :begin-test-run and :summary events. Otherwise everything else about the namespace remains the same.

[miikka]

What I'm basically trying to do is to batch together some of the clojure.test.junit calls to make them work right with multithreading. Let's say you have tests like this:

(deftest happy-test
  (is (= 1 1)))

(deftest sad-test
  (is (= 0 1)))

If you directly use clojure.test.junit with Eftest with :multithread? true, you can end up with XML that looks like this:

<testcase name="sad-test" classname="foo.core-test">
<testcase name="happy-test" classname="foo.core-test">
<failure>expected: (= 0 1)
  actual: (not (= 0 1))
      at: runner.clj:13</failure>
</testcase>
</testcase>

The testcases are weirdly interspersed. Correct output would look like this:

<testcase name="sad-test" classname="foo.core-test">
<failure>expected: (= 0 1)
  actual: (not (= 0 1))
      at: runner.clj:13</failure>
</testcase>
<testcase name="happy-test" classname="foo.core-test">
</testcase>

Now that I try to explain what I'm doing, I realize that my code does not work correctly.

To do right thing, you need to collect the results of all the :pass/:fail/:error calls inside a :begin-test-var/:end-test-var block and emit the XML for all of them at once. This is the purpose of *test-result*. The problem is that my code only works right if there's one such call.

The test results are thread-local data, so push-thread-bindings is kind of convenient, but I imagine the results could be put into *context* as well.

Ripping clojure.test.junit sounds like a good idea, I'll see what I can do tomorrow.

[weavejester]

I see your reasoning for using dynamic vars now. The report method itself is called while synchronized, but the individual test cases can overlap, so recording it at :end-test-var is a good idea.

I suggest using the eftest.report/*context* atom to record the results of each test. I believe you can use (first clojure.test/*testing-vars*) in any report method to find the var currently under test, which you can use as an index.

[miikka]

I've now inlined the code from clojure.test.junit and converted it to use eftest.report/*context*. I threw away the XML pretty-printing, because I don't see much value in it - the XML is supposed to be for machine consumption.

I'm not sure what would be a good solution for passing in the filename for the test report? I used a dynamic variable for now. Or maybe the file opening code should be entirely removed and the user would then need to do something like (binding [*test-out* ...] (junit/report m)) in their report function?

[weavejester]

Thanks! It looks good. I've added a few comments, and I think that binding *test-out* is probably the best solution.

We could later add a function like report-to-file that would wrap a report function and direct the output to a file, e.g.

(report-to-file junit/report "test-out.xml")

However, I think that's a separate PR.

[miikka]

I addressed all your comments.

[weavejester]

Thanks! I'll go and test it later today. Can you squash your commits and change the commit message to:

Add JUnit XML reporter

[weavejester]

I've tested it and I think you're missing references to clojure.test/inc-report-counter, since the summary is returned incorrectly:

user=> (run-tests (find-tests "test") {:report eftest.report.pretty/report})
Ran 1 tests in 0.003 seconds
2 assertions, 0 failures, 0 errors.
{:test 1, :pass 2, :fail 0, :error 0, :type :summary, :duration 3.393087}

user=> (run-tests (find-tests "test") {:report eftest.report.junit/report})
<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
<testsuite name="runner-test" package="eftest"><testcase name="test-reporting" classname="eftest.runner-test"></testcase></testsuite></testsuites>
{:test 1, :pass 0, :fail 0, :error 0, :type :summary, :duration 5.654796}

But other than that, the output seems fine. I tested it with failures, errors and successful test passes.

[miikka]

Right, the counters! Because I intended the JUnit reporter to be used in conjunction with another reporter, I elided inc-report-counter call – only one reporter should call it. Should I add it there?

I started this PR to get an easy-to-use JUnit reporter but maybe what I really need is easy-to-use tools for composing/conjugating reporters.

[weavejester]

Ah, I see what you mean. I think we do need ways of composing reporters - that seems a more general solution than making the JUnit reporter special in some way.

I do notice that the docstring for inc-report-counter is:

Increments the named counter in *report-counters*, a ref to a map. Does nothing if *report-counters* is nil.

So we could do something like:

(fn [m]
  (eftest.report.progress/report m)
  (binding [*report-counters* nil
            *test-out* some-file-stream]
    (eftest.report.junit/report m)))

And of course we could have functions to make composing reporting functions easier.

[miikka]

I amended the squashed commit to add the inc-report-counter calls.

diff

```diff diff --git a/src/eftest/report/junit.clj b/src/eftest/report/junit.clj index 25087a9..b58c28f 100644 --- a/src/eftest/report/junit.clj +++ b/src/eftest/report/junit.clj @@ -117,8 +117,13 @@ (let [test-var (first test/*testing-vars*)] (swap! *context* update-in [::test-results test-var] conj result))) +(defmethod report :pass [m] + (test/inc-report-counter :pass)) + (defmethod report :fail [m] + (test/inc-report-counter :fail) (push-result m)) (defmethod report :error [m] + (test/inc-report-counter :error) (push-result m)) ```

For the record: I noticed that the :test counter value being too small on some runs, but it turned out to be CLJ-1528, which has been fixed in Clojure 1.8 while I was using Clojure 1.7.