should fail gracefully when clojure.test/report is not a multimethod

Description

I don't know if there is a good reason for this to happen, but it has definitely happened a lot.

Environment

None

Activity

Show:
Liam F
November 21, 2017, 6:27 PM

If I'm reading this correctly, this is an issue if someone rebinds test/report with a non-multimethod. Is that correct?

Yes.

Given that the documentation for test/report is that it is a multimethod, it seems that the onus would be on the person rebinding test/report to do so with a multimethod.

The ns docs for clojure.test also say:

You can plug in your own test-reporting framework by rebinding the "report" function: (report event)

Which suggests we can use a function.

Even if one doesn't want to implement any more than a single option, that's just one defmulti and one defmethod.

In my use case it happens inside a non-global scope, with bindings that are only available in that scope. Since there isn't an anonymous multi function, I use fn. I could construct my own Multifn object but that seems like assuming far too much about how this works.

I mean I don't really think it matters, there's code out in the wild that does it. and it's not like what we're trying to do here is going to work, with or without this patch.

A better solution from a "make it work" perspective might be for us to rebind it with a function. That also has the advantage of assuming less about what other users are doing with or to the report function.

Nicolás Berger
November 21, 2017, 7:04 PM

Just to add some context: two prominent places where clojure.test/report gets rebinded to a plain fn are cider-nrepl and vim-fireplace. Links to the relevant code:

https://github.com/clojure-emacs/cider-nrepl/blob/4e23821e57644724d80dea22b83feff60b40e017/src/cider/nrepl/middleware/test.clj#L176

https://github.com/tpope/vim-fireplace/blob/265fc95f7b1b3ec4371403a2f9074dc171bf7978/plugin/fireplace.vim#L1749

gfredericks
November 22, 2017, 2:32 AM

clojure.test/report is a rare case of something that is both a dynamic function and a multimethod, which means you have two different ways to extend it that don't compose well.

How test.check extends the report function has a bearing on whether users can customize it further.

I think the whole situation is too messy to think about redesigning it (without starting a new clojure-test namespace), so I'm mostly trying to stop users from having the awful experience of their code not being able to even compile because of an unfortunate (namespace-load-order-sensitive) interaction between two things they don't understand.

Michael Glaesemann
November 22, 2017, 3:06 AM

Thanks everyone for the detailed explanations. I hope some of this gets into the code to explain the motivation for the gyrations.

gfredericks
November 22, 2017, 12:40 PM

Applied on master (with a comment); thanks everybody!

Completed

Assignee

gfredericks

Reporter

gfredericks

Labels

None

Approval

None

Patch

None

Priority

Major