Use multimethod for cider.nrepl.middleware.test/report

[xiongtx]

Partially address clojure-emacs/cider#2215.

Users should be able to define new message types.

With this change, such new messages will only be handled properly if report produces a map adhering to the schema of that of an existing type (e.g. :fail). This could be another point of extension in the future.

[codecov]

Codecov Report

Merging #510 into master will decrease coverage by 0.05%.
The diff coverage is 35.29%.

@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
- Coverage   72.02%   71.96%   -0.06%     
==========================================
  Files          33       33              
  Lines        2152     2169      +17     
  Branches      112      112              
==========================================
+ Hits         1550     1561      +11     
- Misses        490      496       +6     
  Partials      112      112

Impacted Files	Coverage Δ
src/cider/nrepl/middleware/test.clj	23.29% <35.29%> (+4.42%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8fc3eb...b186ec9. Read the comment docs.

[SevereOverfl0w]

I'm a little uncertain about this PR, because I'm uncertain who would be expected to implement this?

matcher-combinators:: I could see some reluctance from a library author for hooking into more reports. How long before you're supporting N reporters that you've never even heard of? Especially when hooking in will require ugly code which has conditional requires to hook the multi-method.
user:: This is difficult on two fronts, the user has to figure this out & implement it, and somehow hot-load it into their project.

There's an alternative avenue to explore here: cider-nrepl doing the integration directly. This is hindered in this case by matcher-combinators not using a namespaced keyword for the matching. If it did that, we could comfortably implement a matcher for it, as the API doesn't depend on any particular code being loaded.

[bbatsov]

That's a valid point indeed. I often wish we hadn't had to deal with clojure.test in its current state, as there's so much opportunity for improvement there... It is what it is, though.

@claynon Any thoughts on this?

[xiongtx]

CIDER doing integration directly is what we've done w/ integrating pjstadig/humane-test-output's diff functionality.

It's certainly not a sustainable model, though. It'd make CIDER directly or indirectly on a bunch of projects we have no control over. In the case of HTO the functionality'd been stable and demonstrably useful for years, and is a relatively small change.

Making report a multimethod here is a good idea in any case. It splits up the logic on the different types and avoids some awkward code.

[philomates]

This is hindered in this case by matcher-combinators not using a namespaced keyword for the matching.

@SevereOverfl0w do you mean making :mismatch (here) into :matcher-combinators.test/mismatch? If that makes integration with cider easier, I'll happily make that change

@bbatsov I've been thinking a bit about clojure test frameworks (most of my experience is with midje) and I'm curious to hear your (and others') opinions on the trade-offs of clojure.test. I created a comparison chart for midje and clojure.test to figure out what features a new framework should have. If you have insights, it would be cool to hear them either as a comment on the aforementioned chart or maybe on something like clojureverse (I talk a bit about this on this thread)

[claynon]

I don't fully understand the implications of this kind of changes on cider, so I might have a naive expectation.
From what I understand making cider code more extensible (like this PR makes) is a good idea for code isolation. This would give more power to testing libs (like matcher-combinators) and remove the necessity of including code from other libs inside cider.
Also, the approach of extending a multimethod is one that people writing testing libs might be familiar because it's what they need to do if they want to integrate with clojure.test: https://github.com/nubank/matcher-combinators/blob/master/src/matcher_combinators/test.clj#L28-L59

[SevereOverfl0w]

@phillipM I think making that change would make your event more "isolated" in your library. That isolation means cider can integrate with that value, without being concerned about whether or not another library will add a :mismatch and break things.

I think you would also need to commit to not changing your event in a breaking way. If you did, you would need to call it :matcher-combinators.core/mismatch2. (Something something accretion!)

---

From what I understand making cider code more extensible (like this PR makes) is a good idea for code isolation. This would give more power to testing libs (like matcher-combinators) and remove the necessity of including code from other libs inside cider.

There's a difficult balance here. Should matcher-combinators depend on cider-nrepl in order to do the integration? Or should cider-nrepl just look for data that matcher-combinators happens to produce and integrate with that? I'm quite against libraries having specific tooling integrations, as author bias seems to fester there (oh, it just works with cider, use that instead of IntelliJ!).

Using pure data is very beneficial here, as it means there's no dependency cost. But it requires an accretion commitment. (Can we get t-shirts or a certificate or something that has that on?)

[SevereOverfl0w]

Another aspect which occurs to me after posting that comment, is that cider-nrepl's multi-method may be removed in the future, in favour of report generation moving to orchard.

I don't think cider-nrepl is ready to commit to a stable API here, given all the recent refactoring.

[bbatsov]

@bbatsov I've been thinking a bit about clojure test frameworks (most of my experience is with midje) and I'm curious to hear your (and others') opinions on the trade-offs of clojure.test. I created a comparison chart for midje and clojure.test to figure out what features a new framework should have. If you have insights, it would be cool to hear them either as a comment on the aforementioned chart or maybe on something like clojureverse (I talk a bit about this on this thread)

@phillipM Well, we've focused on clojure.test mostly because of its dominant position in the Clojure ecosystem and the fact that we didn't have resources to deal with supporting natively other testing libraries. As a long-time Ruby dev I think RSpec is leaps and bounds better that clojure.test and it provided a lot of inspiration for Midje and Expectations. A general disappointment of mine is that test libraries are rarely written with extensibility or integration with dev tools in mind.

Another aspect which occurs to me after posting that comment, is that cider-nrepl's multi-method may be removed in the future, in favour of report generation moving to orchard.

Yeah, this is likely going to happen soon, but it guess it's not a big deal. Unfortunately coming up with the great and stable APIs everyone desires requires time and energy which no one is willing to commit. :-)

[bbatsov]

Seems to me it'd be a good idea to add some short docstrings to the methods, so people would get an idea what exactly are we handling with them.

[xiongtx]

defmethod doesn't accept a docstring, which is just as well--there's already a docstring for the defmulti, and the methods are just variants that dispatch on :type.

[bbatsov]

Ah, I've forgotten this. Haven't used multimethods in quite a while. My point was mostly that some of the dispatch values might not mean much to casual readers of the code.

[bbatsov]

@xiongtx I'm guessing we should add some integration notes in CIDER's manual, otherwise people are going to have a hard time figuring out what they are supposed to do.

[bbatsov]

Due to lack of further feedback I'll go forward and merge this. @claynon Let us know how this worked out for you.