Skip to content

Sort printed maps in test output #917

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Apr 3, 2025
Merged

Sort printed maps in test output #917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e74c58d
Sort printed maps in test output
sashton Mar 5, 2025
f8cfb95
Provide PR link in CHANGELOG
sashton Mar 5, 2025
027b453
Address list failure
sashton Mar 5, 2025
025658d
Merge branch 'master' into 916-sort-maps-in-printed-test-output
sashton Mar 5, 2025
a6dd36b
Address lint failure
sashton Mar 5, 2025
ec61b70
Remove unneeded try/catch
sashton Mar 5, 2025
e95b9ed
Add comment about the use of deep-sorted-maps
sashton Mar 5, 2025
c446656
Revert "Remove unneeded try/catch"
sashton Mar 12, 2025
ee36478
Add comment explaining try/catch
sashton Mar 12, 2025
12777fd
Update comment
sashton Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* [#913](https://github.com/clojure-emacs/cider-nrepl/pull/913): Disable background warmup of `orchard.java` cache.
* [#913](https://github.com/clojure-emacs/cider-nrepl/pull/913): Enable background warmup of Compliment cache.
* [#914](https://github.com/clojure-emacs/cider-nrepl/pull/914): Remove javadoc section from the inspector output.
* [#917](https://github.com/clojure-emacs/cider-nrepl/pull/917): Sort printed maps in test output

## 0.52.1 (2025-02-24)

Expand Down
21 changes: 20 additions & 1 deletion src/cider/nrepl/middleware/test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,24 @@
(str/starts-with? frame-cname class-name))))
first))))

(defn deep-sorted-maps
"Recursively converts all nested maps to sorted maps."
[m]
(try
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chance for this to throw an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I suppose not. I had that in there when I was first testing it out locally and never removed it. But I don't see it being necessary. I'll remove it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, this will throw if the keys of any nested map are not comparable.

user=> (deep-sorted-maps {{} 1})
Execution error (ClassCastException) at user/deep-sorted-maps$fn (REPL:7).
Default comparator requires nil, Number, or Comparable: {}
user=> (deep-sorted-maps {1 1 :a 1})
Execution error (ClassCastException) at user/deep-sorted-maps$fn (REPL:7).
class java.lang.Long cannot be cast to class clojure.lang.Keyword (java.lang.Long is in module java.base of loader 'bootstrap'; clojure.lang.Keyword is in unnamed module of loader 'app')
user=> (deep-sorted-maps {"a" 1 :a 1})
Execution error (ClassCastException) at user/deep-sorted-maps$fn (REPL:7).
class java.lang.String cannot be cast to class clojure.lang.Keyword (java.lang.String is in module java.base of loader 'bootstrap'; clojure.lang.Keyword is in unnamed module of loader 'app')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frenchy64 That's a great catch! (no pun intended) I'll add a comment explaining the rationale for the try/catch here.

(walk/postwalk
(fn [x]
(if (map? x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't it be better to check around the call site if something's map?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which call site do you mean? If you are referring to the call in print-object, we want to do a postwalk traversal to sort nested maps as well as top-level maps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example nested map in the test:
https://github.com/clojure-emacs/cider-nrepl/pull/917/files#diff-d6825defeb66863e777ca37cd35612c08feadc23a12cab7d9b7d7791f00d407dR285

(with-meta (into (sorted-map) x) (meta x))
x))
m)
(catch Exception _
;; Maps with non-comparable keys aren't sortable so they should be returned as-is.
;; Examples:
;; {{} 1}
;; {1 1 :a 1}
;; {"a" 1 :a 1}
m)))

(defn- print-object
"Print `object` using println for matcher-combinators results and pprint
otherwise. The matcher-combinators library uses a custom print-method
Expand All @@ -91,7 +109,8 @@
print-fn (if matcher-combinators-result?
println
pp/pprint)
result (with-out-str (print-fn object))]
;; The output will contain sorted maps for better readability and diff comparisons.
result (with-out-str (print-fn (deep-sorted-maps object)))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably add a note here why we're sorting maps.

;; Replace extra newlines at the end, as sometimes returned by matchers-combinators:
(str/replace result #"\n\n+$" "\n")))

Expand Down
5 changes: 4 additions & 1 deletion test/clj/cider/nrepl/middleware/test_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,10 @@ The `988` value reflects that it times things correctly for a slow test.")
"pprint is chosen, as indicated by quoted strings and newlines")
(is (= "{:a \"b\", :c \"42\"}\n"
(#'test/print-object (with-meta {:a "b" :c "42"} {:type ::mismatch})))
"pprint is chosen, because :type does not match matchers-combinators keyword")))
"pprint is chosen, because :type does not match matchers-combinators keyword"))
(testing "maps are printed with sorted keys"
(is (= "{:a 1, :b 2, :c 3, :d {x 1, y 2, z 3}}\n"
(#'test/print-object {:b 2 :c 3 :a 1 :d {'z 3 'y 2 'x 1}})))))

(deftest test-result-test
(testing "It passes `:error`s to `test/*test-error-handler*`"
Expand Down