Fix a regression in test results formatting. #713

temochka · 2021-09-17T01:18:16Z

The changes introduced in #683 resulted in test results always being
printed via println instead of clojure.pprint/pprint.

BetterThanTomorrow/calva#1280 describes the degraded user experience
caused by this issue.

temochka · 2021-09-17T01:22:09Z

test/clj/cider/nrepl/middleware/test_test.clj

@@ -152,7 +152,11 @@
    (is (= "custom-output\n"
           (#'test/print-object (with-meta {:not :printed} {:type ::custom}))))
    (is (= "{:a :b, :c :d}\n"


This is a somewhat deceitful example, because a map of symbols printed by println and pprint looks identical:

user=> (println {:a :b :c :d}) {:a :b, :c :d} nil user=> (clojure.pprint/pprint {:a :b :c :d}) {:a :b, :c :d} nil

the differences become apparent once you add strings, symbols, and other data:

user=> (println {:a "string" :c "42" :d 'symbol}) {:a string, :c 42, :d symbol} nil user=> (clojure.pprint/pprint {:a "string" :c "42" :d 'symbol}) {:a "string", :c "42", :d symbol} nil

Nice observation! These tests are good to keep, however please write them in the form (is (= ...) "pprint is chosen, as it can be seen in the quotation marks")

temochka · 2021-09-17T01:23:43Z

src/cider/nrepl/middleware/test.clj

-                        (get-method print-method :default))
-                   pp/pprint
-                   println)]
+  (let [custom-type (:type (meta object))


(:type (meta object)) is clearly nil for any standard Clojure data type and (get-method print-method nil) is never (get-method print-method :default) (it is (get-method print-method nil)).

Can you add a test that explicitly exercises this dispatch?

vemv

Thanks for this one! Looking good.

A few suggestions for shipping this one with full confidence.

vemv · 2021-09-17T01:25:46Z

src/cider/nrepl/middleware/test.clj

-                        (get-method print-method :default))
-                   pp/pprint
-                   println)]
+  (let [custom-type (:type (meta object))


Can you add a test that explicitly exercises this dispatch?

vemv · 2021-09-17T01:27:53Z

src/cider/nrepl/middleware/test.clj

-                   pp/pprint
-                   println)]
+  (let [custom-type (:type (meta object))
+        print-fn (if (and custom-type


I don't understand much what's going on here.

What's a custom-type?

i.e. worth adding a comment

why can custom types result in a vanilla println?

I'm not sure of how 'customness' relates to the choice of println.

See #683 for more details on that. I’m not sure myself, to be honest. I don’t understand how exactly the :type meta attribute gets assigned to a value, but I don’t want to repeat #683’s mistakes and break the formatting for that case. Basically, my goal here is to keep the existing tests passing.

vemv · 2021-09-17T01:29:31Z

test/clj/cider/nrepl/middleware/test_test.clj

@@ -152,7 +152,11 @@
    (is (= "custom-output\n"
           (#'test/print-object (with-meta {:not :printed} {:type ::custom}))))
    (is (= "{:a :b, :c :d}\n"


Nice observation! These tests are good to keep, however please write them in the form (is (= ...) "pprint is chosen, as it can be seen in the quotation marks")

vemv · 2021-09-17T01:30:17Z

test/clj/cider/nrepl/middleware/test_test.clj

+           (#'test/print-object {:a :b :c :d})))
+    (is (= "{:a \"b\", :c \"d\"}\n"
+           (#'test/print-object {:a "b" :c "d"})))
+    (is (= "\"string\"\n"


I'd add another case in which a \n can be seen. That makes it more even more evident that pprint is being used.

vemv · 2021-09-17T01:32:28Z

test/clj/cider/nrepl/middleware/test_test.clj

+    (is (= "{:a \"b\", :c \"d\"}\n"
+           (#'test/print-object {:a "b" :c "d"})))
+    (is (= "\"string\"\n"
+           (#'test/print-object "string")))))


As hinted in Can you add a test that explicitly exercises this dispatch? I'd suggest that one also tests that one can get a vanilla println by passing the right option.

IOW, I should see a test case for {:a string, :c 42, :d symbol} and another for {:a "string", :c "42", :d symbol} per https://github.com/clojure-emacs/cider-nrepl/pull/713/files#r710635100

temochka · 2021-09-17T02:08:26Z

@vemv I made the changes that you requested, attempting to preserve the logic added in #683 unchanged while fixing the regression. That said, if you want my honest opinion: #683 should be completely reverted. I don’t understand that change, and I have a suspicion that it doesn’t work at all. That :type meta attribute seems made up: print-method dispatches based on value type, not metadata. E.g., this works as I expect:

(= (get-method print-method "foo")
   (get-method print-method :default))
; => true

(= (get-method print-method {})
   (get-method print-method :default))
; => true

maybe the below would work instead, but if it’s in fact broken, I’d rather revert than try to fix something that has been broken and nobody complained 🤷‍♂️

(defn- print-object
  "Print `object` using pprint or a custom print-method, if available."
  [object]
  (let [print-fn (if (not= (get-method print-method object)
                           (get-method print-method :default))
                   println
                   pp/pprint)]
    (with-out-str (print-fn object))))

vemv · 2021-09-17T02:27:05Z

Thanks for the iteration! Looking great.

Taking a quick look at the #683 rationale, it seems a good idea to try exercising matchers-combinators or at least try getting an idea of how matchers-combinators prints things, how #683 intended to handle it, etc.

i.e. if we can ensure the matchers-combinators integration will keep working (or identify that it wasn't working properly), so much the bettter.

We can always ping the author in question @FelipeCortez (done!)

temochka · 2021-09-17T03:40:03Z

@vemv I spent a little more time looking into this and turned out I was wrong about this:

print-method dispatches based on value type, not metadata

Example:

(defmethod clojure.core/print-method ::custom [_ out]
  (binding [*out* out]
    (print "custom-output")))

(println (with-meta {:foo "bar"} {:type ::custom}))

; custom output
; nil

Overall, I don’t feel like I have enough context (or proper motivation) to propose changes to existing matchers-combinators support. This PR fixes an issue affecting the majority of cider-nrepl users while satisfying the tests (and preserving the logic) provided for matchers-combinators support. To me, it sounds like a win-win. Can we merge it and track any potential matchers-combinators issues separately?

vemv · 2021-09-17T07:16:45Z

Hi,

Can we merge it and track any potential matchers-combinators issues separately?

I don't think so, we can't be casually wrong about things and still go ahead with our original plans. A great PR will fix things for everyone.

If you'd like to give the complete task a shot let us know!

Cheers - V

vemv

This PR is on the right track, following BetterThanTomorrow/calva#794 I can relate that issue to weird output formats I've seen in certain CIs when tests fail.

Suggesting a simple actionable thing for the test coverage, after that it would be probably ready.

Maybe we'd need an integration test that can't be fooled so easily, but that can be tackled separately.

vemv · 2021-09-17T07:55:23Z

test/clj/cider/nrepl/middleware/test_test.clj

+           (#'test/print-object (with-meta {:a "b" :c "42"} {:type {}})))
+        "pprint is chosen, because :type is printed via :default print-method")
+    (is (= "{:a b, :c 42}\n"
+           (#'test/print-object (with-meta {:a "b" :c "42"} {:type Number})))


A more realistic test setting would involve defining a print-method for e.g. a defrecord TestRecord and asserting that that print-method is invoked, instead of vanilla println.

temochka · 2021-09-18T04:14:16Z

A great PR will fix things for everyone.

@vemv fair enough. I’m going on vacation until later next week so don‘t expect any updates in the meantime. I’ll be thinking about the following things (feedback welcome):

is the reference check against (get-method print-method :default) sufficient for detecting a user-defined print-method?
does Support custom print-methods in test reports #683 serve exclusively matcher-combinators users or there are indeed other libs that rely on this?
related to ☝️, can we target specifically matcher-combinators metadata?
does it make sense to use println (discards string quotes, generally intended for user-facing values) and not prn, which would at least print values in a way that can be fed back to REPL?

vemv · 2021-09-18T04:24:38Z

Cheers. I wonder if we could ask the matchers-combinators authors to use something ns-qualified in addition / instead of :type if that helps?

bbatsov · 2021-10-18T06:27:02Z

ping :-)

temochka · 2021-10-20T16:17:31Z

@vemv @bbatsov sorry for disappearing.

I wonder if we could ask the matchers-combinators authors to use something ns-qualified in addition / instead of :type if that helps?

Right, this is where I arrived at as well and it’s already possible. I changed the PR to limit println usage to matcher-combinators results only by relying on the namespaced keyword used as :type by that lib (:matcher-combinators.clj-test/mismatch). I updated the automated tests and performed manual integration testing with my local snapshot of cider-nrepl using the following test:

(ns calva-bug.core-test
  (:require [clojure.test :refer [deftest is]]
            [calva-bug.core :refer :all]
            [matcher-combinators.test]
            [matcher-combinators.matchers :as m]))

(deftest a-test
  (is (= {:foo "bar"} {:foo "buz"})))

(deftest test-matching-with-explicit-matchers
  (is (match? (m/equals 37) (+ 29 3)))
  (is (match? (m/regex #"fox") "The quick brown fox jumps over the lazy dog")))

(deftest test-matching-scalars
  ;; most scalar values are interpreted as an `equals` matcher
  (is (match? 37 (+ 29 8)))
  (is (match? "this string" (str "this" " " "string")))
  (is (match? :this/keyword (keyword "this" "keyword")))
  ;; regular expressions are handled specially
  (is (match? #"fox" "The quick brown fox jumps over the lazy dog")))

Test output:

; Running tests for calva-bug.core-test...
; FAIL in calva-bug.core-test/a-test (core_test.clj:8):
; expected:
{:foo "bar"}
; actual:
({:foo "buz"})
; FAIL in calva-bug.core-test/test-matching-with-explicit-matchers (core_test.clj:11):
; expected:
(match? (m/equals 37) (+ 29 3))
; actual:
(mismatch (expected 37) (actual 32))

; 7 tests finished, problems found. 😭 errors: 0, failures: 2, ns: 1, vars: 3

Looks like both clojure.test and matcher-combinators examples print the expected output.

I should add that this is a potentially breaking change for other hypothetical test libs that rely on print-method. That said:

detecting a custom print-method via a reference check (as in Support custom print-methods in test reports #683) seems fragile and likely has false-positives (there are many print-method definitions that are not :default just in clojure.core)
it’s not a given that the use of a user-defined print-method in contexts where pprint is used is desirable. I believe, libraries can hook into pprint dispatch if needed.
I’m not aware of other libs (and Support custom print-methods in test reports #683 doesn't provide examples) that use the print-method trick so the existing approach may not even be generalizable.

vemv

Thanks for the update!

Looks like we can aim for this to be merged for the upcoming major release.

src/cider/nrepl/middleware/test.clj

The changes introduced in clojure-emacs#683 resulted in test results always being printed via println instead of clojure.pprint/pprint. BetterThanTomorrow/calva#1280 describes the degraded user experience caused by this issue. This PR limits println usage to matchers-combinators results.

vemv · 2021-10-21T12:59:30Z

LGTM, it's fairly clear that this PR reverts kind of a dubious prior PR adding more coverage.

Please add a changelog entry and we're good to go 🍻

PEZ · 2021-10-21T14:52:31Z

🎉

temochka · 2021-10-21T20:01:56Z

Please add a changelog entry and we're good to go

@vemv @bbatsov looks like this got merged before I got a chance to do that. Will y’all update the changelog or you want me to send another PR?

vemv · 2021-10-21T21:08:25Z

Appreciated!

A PR would be nice briefly summarizing the impact of the change for anyone that might have relied on the transient behavior we had (which seems unlikely)

bbatsov · 2021-10-22T05:24:07Z

I added a basic changelog entry myself. I don't think there's any need to elaborate on this change further, given we considered it a bugfix.

vemv · 2021-12-07T12:54:20Z

Released in [cider/cider-nrepl "0.27.3"]!

temochka commented Sep 17, 2021

View reviewed changes

vemv reviewed Sep 17, 2021

View reviewed changes

temochka force-pushed the ac/fix-test-results-printing branch from a7de95c to a096e57 Compare September 17, 2021 01:59

vemv added the not ready for merging label Sep 17, 2021

vemv marked this pull request as draft September 17, 2021 07:35

vemv reviewed Sep 17, 2021

View reviewed changes

temochka force-pushed the ac/fix-test-results-printing branch from a096e57 to 4519f16 Compare October 20, 2021 15:49

temochka marked this pull request as ready for review October 20, 2021 16:17

vemv reviewed Oct 20, 2021

View reviewed changes

src/cider/nrepl/middleware/test.clj Show resolved Hide resolved

temochka force-pushed the ac/fix-test-results-printing branch from 4519f16 to 01a7002 Compare October 20, 2021 18:44

vemv mentioned this pull request Oct 21, 2021

Expected/actual values in REPL-run tests are not valid Clojure data BetterThanTomorrow/calva#1280

Open

vemv removed the not ready for merging label Oct 21, 2021

vemv approved these changes Oct 21, 2021

View reviewed changes

bbatsov merged commit dff71aa into clojure-emacs:master Oct 21, 2021

temochka deleted the ac/fix-test-results-printing branch October 21, 2021 20:01

bbatsov added a commit that referenced this pull request Oct 22, 2021

[#713] Add changelog entry

f773382

vemv mentioned this pull request Dec 9, 2021

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a regression in test results formatting. #713

Fix a regression in test results formatting. #713

temochka commented Sep 17, 2021

temochka Sep 17, 2021

vemv Sep 17, 2021

temochka Sep 17, 2021

vemv Sep 17, 2021

vemv left a comment

vemv Sep 17, 2021

vemv Sep 17, 2021

temochka Sep 17, 2021

vemv Sep 17, 2021

vemv Sep 17, 2021

vemv Sep 17, 2021

temochka commented Sep 17, 2021

vemv commented Sep 17, 2021

temochka commented Sep 17, 2021

vemv commented Sep 17, 2021

vemv left a comment

vemv Sep 17, 2021

temochka commented Sep 18, 2021

vemv commented Sep 18, 2021

bbatsov commented Oct 18, 2021

temochka commented Oct 20, 2021

vemv left a comment

vemv commented Oct 21, 2021

PEZ commented Oct 21, 2021

temochka commented Oct 21, 2021

vemv commented Oct 21, 2021

bbatsov commented Oct 22, 2021

vemv commented Dec 7, 2021

Fix a regression in test results formatting. #713

Fix a regression in test results formatting. #713

Conversation

temochka commented Sep 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

temochka commented Sep 17, 2021

vemv commented Sep 17, 2021

temochka commented Sep 17, 2021

vemv commented Sep 17, 2021

vemv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

temochka commented Sep 18, 2021

vemv commented Sep 18, 2021

bbatsov commented Oct 18, 2021

temochka commented Oct 20, 2021

vemv left a comment

Choose a reason for hiding this comment

vemv commented Oct 21, 2021

PEZ commented Oct 21, 2021

temochka commented Oct 21, 2021

vemv commented Oct 21, 2021

bbatsov commented Oct 22, 2021

vemv commented Dec 7, 2021