Adopt underused buttercup facilities in the test suite

An audit against the buttercup docs found features we weren't using:

- assume: environment-dependent specs (libxml, cygwin paths) now report
  as skipped instead of silently asserting a fallback branch.
- Custom matchers in cider-test-utils: :to-have-sent-op collapses the
  spy-and-pluck plumbing around nREPL request assertions and names the
  offending key on failure; :to-equal-dict compares nrepl-dicts
  order-insensitively and reports differing keys.
- spy-calls-most-recent instead of indexed spy-calls-args-for, which
  breaks when a function gains an earlier call.

The hacking guide documents the conventions.

Author: bbatsov

doc/modules/ROOT/pages/contributing/hacking.adoc

@@ -80,6 +80,19 @@ the
 https://github.com/jorgenschaefer/emacs-buttercup/blob/master/docs/running-tests.md[buttercup documentation] for
 all the details.
 
+A few suite conventions worth knowing:
+
+* Specs that only make sense in certain environments (a specific OS, Emacs
+  feature or library) should declare that with buttercup's `assume`, so they
+  show up as skipped instead of silently passing over the interesting branch.
+* `test/utils/cider-test-utils.el` defines custom matchers for the common
+  assertion patterns: `:to-have-sent-op` for checking the nREPL request a
+  spied request function sent, and `:to-equal-dict` for order-insensitive
+  nrepl-dict comparison with key-level failure messages.  Prefer them over
+  hand-rolled request/dict plumbing.
+* Prefer `spy-calls-most-recent` over indexed `spy-calls-args-for` - it
+  doesn't break when a function gains an earlier internal call.
+
 ==== Running the tests in batch mode
 
 If you prefer running all tests outside Emacs that's also an option.

test/cider-find-tests.el

@@ -63,13 +63,13 @@
             (nrepl-dict "dest" (current-buffer) "dest-point" 1))
     (spy-on 'cider-jump-to)
     (cider-find-keyword '(4))           ; single C-u -> same window
-    (expect (nth 2 (spy-calls-args-for 'cider-jump-to 0)) :to-be nil)
+    (expect (nth 2 (spy-context-args (spy-calls-most-recent 'cider-jump-to))) :to-be nil)
     (spy-calls-reset 'cider-jump-to)
     (cider-find-keyword '(16))          ; double C-u -> other window
-    (expect (nth 2 (spy-calls-args-for 'cider-jump-to 0)) :to-be-truthy)
+    (expect (nth 2 (spy-context-args (spy-calls-most-recent 'cider-jump-to))) :to-be-truthy)
     (spy-calls-reset 'cider-jump-to)
     (cider-find-keyword '-)             ; negative -> other window
-    (expect (nth 2 (spy-calls-args-for 'cider-jump-to 0)) :to-be-truthy)))
+    (expect (nth 2 (spy-context-args (spy-calls-most-recent 'cider-jump-to))) :to-be-truthy)))
 
 (describe "cider--find-keyword-loc"
   (it "finds the given keyword, discarding false positives"

test/cider-interaction-tests.el

@@ -45,19 +45,21 @@
 
 (describe "cider-to-nrepl-filename-function"
   (it "translates file paths when running on cygwin systems"
+    (assume (eq system-type 'cygwin) "not running on cygwin")
     (let ((windows-file-name "C:/foo/bar")
           (unix-file-name "/cygdrive/c/foo/bar"))
-      (if (eq system-type 'cygwin)
-          (progn
-            (expect (funcall cider-from-nrepl-filename-function windows-file-name)
-                    :to-equal unix-file-name)
-            (expect (funcall cider-to-nrepl-filename-function unix-file-name)
-                    :to-equal windows-file-name))
-        (progn
-          (expect (funcall cider-from-nrepl-filename-function unix-file-name)
-                  :to-equal unix-file-name)
-          (expect (funcall cider-to-nrepl-filename-function unix-file-name)
-                  :to-equal unix-file-name)))))
+      (expect (funcall cider-from-nrepl-filename-function windows-file-name)
+              :to-equal unix-file-name)
+      (expect (funcall cider-to-nrepl-filename-function unix-file-name)
+              :to-equal windows-file-name)))
+
+  (it "leaves file paths alone on other systems"
+    (assume (not (eq system-type 'cygwin)) "running on cygwin")
+    (let ((unix-file-name "/cygdrive/c/foo/bar"))
+      (expect (funcall cider-from-nrepl-filename-function unix-file-name)
+              :to-equal unix-file-name)
+      (expect (funcall cider-to-nrepl-filename-function unix-file-name)
+              :to-equal unix-file-name)))
   (it "translates file paths from container/vm location to host location"
     (let* ((/docker/src (expand-file-name "/docker/src"))
            (/cygdrive/c/project/src (expand-file-name "/cygdrive/c/project/src"))

test/cider-log-tests.el

@@ -27,6 +27,7 @@
 
 (require 'buttercup)
 (require 'cider-log)
+(require 'cider-test-utils)
 
 ;; Please, for each `describe', ensure there's an `it' block, so that its execution is visible in CI.
 
@@ -200,10 +201,6 @@
       (expect (substring-no-properties (cider-log--description-clear-events-buffer))
               :to-equal "Clear *cider-log* buffer"))))
 
-(defun cider-log-tests--request-get (request key)
-  "Return the value following KEY in the nREPL REQUEST list."
-  (cadr (member key request)))
-
 (describe "cider-log nREPL request construction"
   :var (framework appender event)
   (before-each
@@ -219,24 +216,17 @@
             :and-return-value (nrepl-dict "cider/log-add-appender" appender))
     (expect (cider-sync-request:log-add-appender framework appender)
             :to-equal appender)
-    (let ((request (car (spy-calls-args-for 'cider-nrepl-sync-request 0))))
-      (expect (cider-log-tests--request-get request "op")
-              :to-equal "cider/log-add-appender")
-      (expect (cider-log-tests--request-get request "framework") :to-equal "logback")
-      (expect (cider-log-tests--request-get request "appender") :to-equal "my-appender")
-      (expect (cider-log-tests--request-get request "size") :to-equal 100)
-      (expect (cider-log-tests--request-get request "threshold") :to-equal 10)
-      (expect (nrepl-dict-get (cider-log-tests--request-get request "filters") "level")
-              :to-equal "INFO")))
+    (expect 'cider-nrepl-sync-request :to-have-sent-op "cider/log-add-appender"
+            "framework" "logback" "appender" "my-appender"
+            "size" 100 "threshold" 10)
+    (expect (cider-request-get (cider-sent-request 'cider-nrepl-sync-request) "filters")
+            :to-equal-dict (nrepl-dict "level" "INFO")))
 
   (it "cider-sync-request:log-clear sends the clear-appender op"
     (spy-on 'cider-nrepl-sync-request :and-return-value (nrepl-dict))
     (cider-sync-request:log-clear framework appender)
-    (let ((request (car (spy-calls-args-for 'cider-nrepl-sync-request 0))))
-      (expect (cider-log-tests--request-get request "op")
-              :to-equal "cider/log-clear-appender")
-      (expect (cider-log-tests--request-get request "framework") :to-equal "logback")
-      (expect (cider-log-tests--request-get request "appender") :to-equal "my-appender")))
+    (expect 'cider-nrepl-sync-request :to-have-sent-op "cider/log-clear-appender"
+            "framework" "logback" "appender" "my-appender"))
 
   (it "cider-sync-request:log-search sends the filters and pagination"
     (spy-on 'cider-nrepl-sync-request
@@ -245,56 +235,42 @@
       (expect (cider-sync-request:log-search framework appender
                                              :filters filters :limit 25 :offset 5)
               :to-equal (list event))
-      (let ((request (car (spy-calls-args-for 'cider-nrepl-sync-request 0))))
-        (expect (cider-log-tests--request-get request "op")
-                :to-equal "cider/log-search")
-        (expect (cider-log-tests--request-get request "filters") :to-equal filters)
-        (expect (cider-log-tests--request-get request "limit") :to-equal 25)
-        (expect (cider-log-tests--request-get request "offset") :to-equal 5))))
+      (expect 'cider-nrepl-sync-request :to-have-sent-op "cider/log-search"
+              "filters" filters "limit" 25 "offset" 5)))
 
   (it "cider-sync-request:log-inspect-event asks for the event by id"
     (spy-on 'cider-nrepl-sync-request
             :and-return-value (nrepl-dict "value" "inspected"))
     (expect (cider-sync-request:log-inspect-event framework appender event)
             :to-equal "inspected")
-    (let ((request (car (spy-calls-args-for 'cider-nrepl-sync-request 0))))
-      (expect (cider-log-tests--request-get request "op")
-              :to-equal "cider/log-inspect-event")
-      (expect (cider-log-tests--request-get request "event") :to-equal "ev-42")))
+    (expect 'cider-nrepl-sync-request :to-have-sent-op "cider/log-inspect-event"
+            "event" "ev-42"))
 
   (it "cider-sync-request:log-format-event includes the print options"
     (spy-on 'cider-nrepl-sync-request
             :and-return-value (nrepl-dict "cider/log-format-event" "formatted"))
     (expect (cider-sync-request:log-format-event framework appender event)
             :to-equal "formatted")
-    (let ((request (car (spy-calls-args-for 'cider-nrepl-sync-request 0))))
-      (expect (cider-log-tests--request-get request "op")
-              :to-equal "cider/log-format-event")
-      (expect (cider-log-tests--request-get request "event") :to-equal "ev-42")
-      (expect (cider-log-tests--request-get request "nrepl.middleware.print/stream?")
-              :to-equal "1")))
+    (expect 'cider-nrepl-sync-request :to-have-sent-op "cider/log-format-event"
+            "event" "ev-42" "nrepl.middleware.print/stream?" "1"))
 
   (it "cider-request:log-add-consumer sends the consumer's filters and the callback"
     (spy-on 'cider-nrepl-send-request)
     (let ((consumer (nrepl-dict "filters" (nrepl-dict "level" "INFO"))))
       (cider-request:log-add-consumer framework appender consumer #'ignore)
-      (let ((args (spy-calls-args-for 'cider-nrepl-send-request 0)))
-        (expect (cider-log-tests--request-get (car args) "op")
-                :to-equal "cider/log-add-consumer")
-        (expect (cider-log-tests--request-get (car args) "framework") :to-equal "logback")
-        (expect (cider-log-tests--request-get (car args) "appender") :to-equal "my-appender")
-        (expect (nrepl-dict-get (cider-log-tests--request-get (car args) "filters") "level")
+      (expect 'cider-nrepl-send-request :to-have-sent-op "cider/log-add-consumer"
+              "framework" "logback" "appender" "my-appender")
+      (let ((args (spy-context-args (spy-calls-most-recent 'cider-nrepl-send-request))))
+        (expect (nrepl-dict-get (cider-request-get (car args) "filters") "level")
                 :to-equal "INFO")
         (expect (cadr args) :to-be #'ignore))))
 
   (it "cider-sync-request:log-remove-consumer identifies the consumer by id"
     (spy-on 'cider-nrepl-sync-request :and-return-value (nrepl-dict))
     (cider-sync-request:log-remove-consumer
      framework appender (nrepl-dict "id" "my-consumer"))
-    (let ((request (car (spy-calls-args-for 'cider-nrepl-sync-request 0))))
-      (expect (cider-log-tests--request-get request "op")
-              :to-equal "cider/log-remove-consumer")
-      (expect (cider-log-tests--request-get request "consumer") :to-equal "my-consumer"))))
+    (expect 'cider-nrepl-sync-request :to-have-sent-op "cider/log-remove-consumer"
+            "consumer" "my-consumer")))
 
 (describe "cider-log appender and consumer accessors"
   (it "reads the appender size, threshold, filters and consumers"
@@ -454,6 +430,6 @@
             (cider-log-appender-id "my-appender"))
         (cider-log-info)))
     (expect (substring-no-properties
-             (apply #'format (spy-calls-args-for 'message 0)))
+             (apply #'format (spy-context-args (spy-calls-most-recent 'message))))
             :to-equal
             "Buffer: *cider-log* Framework: Logback Appender: my-appender Consumer: c1")))

test/cider-stacktrace-tests.el

@@ -282,7 +282,7 @@
                                         'line 12)))
         (cider-stacktrace-navigate button)
         (expect 'cider-var-info :not :to-have-been-called)
-        (let ((info (car (car (spy-calls-all-args 'cider--jump-to-loc-from-info)))))
+        (let ((info (car (spy-context-args (spy-calls-most-recent 'cider--jump-to-loc-from-info)))))
           (expect (nrepl-dict-get info "file") :to-equal "file:/tmp/repro.clj")
           (expect (nrepl-dict-get info "line") :to-equal 12)))))
 

test/cider-util-tests.el

@@ -605,7 +605,12 @@ and some other vars (like clojure.core/filter).
 
 (describe "cider--render-html-string"
   (it "renders html to plain text when libxml is available"
-    (if (and (fboundp 'libxml-available-p) (libxml-available-p))
-        (expect (substring-no-properties (cider--render-html-string "<p><b>hello</b></p>"))
-                :to-equal "hello")
-      (expect (cider--render-html-string "<b>x</b>") :to-equal "<b>x</b>"))))
+    (assume (and (fboundp 'libxml-available-p) (libxml-available-p))
+            "libxml support not available")
+    (expect (substring-no-properties (cider--render-html-string "<p><b>hello</b></p>"))
+            :to-equal "hello"))
+
+  (it "returns the raw html without libxml support"
+    (assume (not (and (fboundp 'libxml-available-p) (libxml-available-p)))
+            "libxml support is available")
+    (expect (cider--render-html-string "<b>x</b>") :to-equal "<b>x</b>")))

test/utils/cider-test-utils.el

@@ -25,7 +25,12 @@
 
 ;;; Code:
 
+(require 'buttercup)
+(require 'cl-lib)
 (require 'clojure-mode)
+(require 'nrepl-dict)
+(require 'seq)
+(require 'subr-x)
 
 (defun with-clojure-buffer--go-to-point ()
   "Move point to the `|' marker and delete it, if present."
@@ -46,6 +51,85 @@ buffer."
      (with-clojure-buffer--go-to-point)
      ,@body))
 
+
+;;; nREPL request assertions
+
+(defun cider-request-get (request key)
+  "Return the value following KEY in the nREPL REQUEST list."
+  (cadr (member key request)))
+
+(defun cider-sent-request (spy)
+  "Return the request (first argument) of the most recent call to SPY."
+  (when-let* ((context (spy-calls-most-recent spy)))
+    (car (spy-context-args context))))
+
+(buttercup-define-matcher :to-have-sent-op (spy op &rest expected)
+  "Match when SPY's most recent request carries the op OP.
+EXPECTED are additional key/value pairs the request must contain,
+compared with `equal'.  Use it on a spied request function:
+
+  (expect \\='cider-nrepl-sync-request :to-have-sent-op \"cider/log-search\"
+          \"framework\" \"logback\" \"limit\" 25)"
+  (let* ((spy (funcall spy))
+         (op (funcall op))
+         (expected (mapcar #'funcall expected))
+         (request (cider-sent-request spy)))
+    (cond
+     ((null request)
+      (cons nil (format "Expected `%s' to have sent op %S, but it was never called"
+                        spy op)))
+     ((not (equal op (cider-request-get request "op")))
+      (cons nil (format "Expected `%s' to have sent op %S, but its last request was %S"
+                        spy op request)))
+     (t
+      (let (mismatches)
+        (cl-loop for (key value) on expected by #'cddr
+                 for actual = (cider-request-get request key)
+                 unless (equal value actual)
+                 do (push (format "%S is %S (expected %S)" key actual value)
+                          mismatches))
+        (if mismatches
+            (cons nil (format "Expected `%s's %S request to match, but %s"
+                              spy op (string-join (nreverse mismatches) ", ")))
+          (cons t (format "Expected `%s' not to have sent op %S matching %S, but it did"
+                          spy op expected))))))))
+
+(defun cider-test-utils--dict-pairs (dict)
+  "Return DICT's entries as an alist."
+  (let (pairs)
+    (nrepl-dict-map (lambda (k v) (push (cons k v) pairs)) dict)
+    (nreverse pairs)))
+
+(buttercup-define-matcher :to-equal-dict (a b)
+  "Match when the nREPL dicts A and B contain `equal' entries.
+Unlike `:to-equal', key order is irrelevant and the failure message
+reports the differing keys instead of printing both dicts whole."
+  (let* ((a (funcall a))
+         (b (funcall b))
+         (a-pairs (cider-test-utils--dict-pairs a))
+         (b-pairs (cider-test-utils--dict-pairs b))
+         (missing (seq-remove (lambda (pair) (assoc (car pair) a-pairs)) b-pairs))
+         (extra (seq-remove (lambda (pair) (assoc (car pair) b-pairs)) a-pairs))
+         (differing (seq-filter (lambda (pair)
+                                  (let ((other (assoc (car pair) b-pairs)))
+                                    (and other (not (equal (cdr pair) (cdr other))))))
+                                a-pairs)))
+    (if (or missing extra differing)
+        (cons nil (string-join
+                   (delq nil
+                         (list (when missing
+                                 (format "missing keys: %S" (mapcar #'car missing)))
+                               (when extra
+                                 (format "unexpected keys: %S" (mapcar #'car extra)))
+                               (when differing
+                                 (mapconcat (lambda (pair)
+                                              (format "%S is %S (expected %S)"
+                                                      (car pair) (cdr pair)
+                                                      (cdr (assoc (car pair) b-pairs))))
+                                            differing ", "))))
+                   "; "))
+      (cons t (format "Expected dicts to differ, but both equal %S" a)))))
+
 (provide 'cider-test-utils)
 
 ;;; cider-test-utils.el ends here