When a test fails, there is a set of parens around the actual result

[marcomorain]

Moved from nrepl/nrepl#260

Expected behaviour

When a test fails in directly in Clojure, I get the following output:

$ clj
Clojure 1.10.3
user=> (require '[clojure.test :refer :all])
nil
user=> (is (= "a" \a))

FAIL in () (NO_SOURCE_FILE:1)
expected: (= "a" \a)
  actual: (not (= "a" \a))
false

Actual behavior

When I run the same test through a Cider nREPL, I get the following output, (this is taken from a Calva REPL):

clj꞉sample-project.core-test꞉> (require '[clojure.test :refer :all])
nil
clj꞉sample-project.core-test꞉> (is (= "a" \a))

FAIL in () (form-init17836416246985520428.clj:8)
expected: "a"
  actual: (\a)
false

The key difference here is that Clojure shows that the expected result was \a, but nREPL says the actual result was (\a).

This is confusing. It makes it difficult to debug why a test has failed, because it appears like the actual result is a list of \a, (i.e. '(\a)), rather that \a. This also makes it very difficult to copy+paste results into tests expectations.

Steps to reproduce the problem

Evaluate the following two forms in a nREPL:

(require '[clojure.test :refer :all])
(is (= "a" \a))

I see the following in the Calva nREPL log:

<- received
{
  id: '80',
  out: 'expected: "a"\n',
  session: 'd3f9f96c-2c5a-4e89-806d-0b86f1ab9e6b'
}

<- received
{
  id: '80',
  out: '  actual: (\\a)\n',
  session: 'd3f9f96c-2c5a-4e89-806d-0b86f1ab9e6b'
}

Which shows that the string is being produced server-side, and is not being altered by Calva.

Environment & Version information

cider-nrepl version

cider-nrepl 0.27.3
nrepl 0.8.3

Java version

openjdk version "16.0.1" 2021-04-20
OpenJDK Runtime Environment AdoptOpenJDK-16.0.1+9 (build 16.0.1+9)
OpenJDK 64-Bit Server VM AdoptOpenJDK-16.0.1+9 (build 16.0.1+9, mixed mode, sharing)

Operating system

macOS 12.0.1 (21A559)

[vemv]

Rings a bell, we tried to improve this area recently (which probably didn't worsen anything, but might not have fixed all cases).

We'll take a look!

[marcomorain]

I've found the cause of the issue – cider.nrepl.middleware.test.extensions/=-body .

cider-nrepl adds a new handler for clojure.test/assert-expr to handle =. This handler is passed the arguments to =.

If you macroexpand (is (= 1 2)), more will be the list (2). If you expand (is (= 1 2 3 4)), more will be (2 3 4).

This results in this behaviour:

(deftest cider-nrepl-behaviour
  (testing "2-artity"
    (is (= 1 2)))
  (testing "arity-3"
    (is (= 1 2 3))))

; Running test: cider-nrepl-behaviour…
; FAIL in core/cider-nrepl-behaviour (core.clj:20):
; 2-artity
; expected:
1
; actual:
(2)
; FAIL in core/cider-nrepl-behaviour (core.clj:22):
; arity-3
; expected:
1
; actual:
(2 3)
; 2 tests finished, problems found. 😭 errors: 0, failures: 2, ns: 1, vars: 1

[vemv]

Thanks for that!

That might be only half of the story though. Note that that ns hasn't changed over 2 years.

What has changed recently is the way of printing test results.

Will take a look at both areas 👀

[vemv]

(For clarity, I'm talking about #713)

[vemv]

Ok, so

• I can reproduce this
• Fix a regression in test results formatting. #713 finally has nothing to do
  • i.e. I checked out the repo before that commit, made no difference

which means this bug has existed as-is for a couple years at least. (I verified so just now by reviewing https://github.com/clojure-emacs/cider-nrepl/commits/master/src/cider/nrepl/middleware/test/extensions.clj)

Probably it has been unnoticed because a cider-nrepl based connection willl typically run tests through cider, rather than by hitting clojure.test directly?

Anyway this is an unfortunate situation, mutating clojure.test globally seems pretty far from ideal.

Maybe we can refactor things to not touch clojure.test multimethods while preserving the desired behavior?

For example here we define our own multimethod:

cider-nrepl/src/cider/nrepl/middleware/test.clj

Lines 142 to 149 in 338023f

	(defmulti report
	"Handle reporting for test events.
	This takes a test event map as an argument and updates the `current-report`
	atom to reflect test results and summary statistics."
	:type)
	(defmethod report :default [_m])

That model seems cleaner.

[vemv]

Maybe instead of making the extension not-global, we could just fix it to handle & correctly.

Hopefully getting its intended diffing behavior reasonably right won't be too much of a hassle.