Introduce integration tests

1. Migrated macos regression tests to circle ci, but with a reduced set to only cover Emacs latest version.
2. Replaced existing macos only testing github action to run the integration tests across macos, ubuntu and windows latest on Emacs 28, 27 and 26. It was nearly impossible to do so on circleci across all platforms, thus a github action was chosen.
3. Updated the `Eldev` project file to support integration test via a new `--test-type` command line option (the template was taken from the `Eldev` tool's project file itself).
4. The cider server is given now a 0.5 sec opportunity to execute the `close` op before the server is killed.
5. The hacking section in documentation has been updated to mention integration tests.
6. The `nrepl--kill-process` has been updated to address the orphaned child process menace under MS-Windows, whereby the nREPL child processes were most likely to be left orphaned after the parent process was killed. The kill is now done by contracting the external `taskkill` windows program to do the job if available, and falls back to the previous logic otherwise.
7. The `nrepl-server-sentinel` has been simplified to only report an error if the nREPL process couldn't bootstrap itself, or send an exit message otherwise. The old code only closed client connections on `hangup` (i.e. HUP signal), but the new logic will close clients on any fatal signal. Not sure why the old code only did this on HUP, but it seems sensible to me that any open client connection should close when the server has exited.
8. Fixed an issue in `cider-tests.el` that a couple of buffer local shadow-cljs variables were set as global by mistake, thus affecting other tests (and in particular the integration tests).
9. Updated nrepl-client-tests.el to test the new nrepl plist property to indicate that the nREPL server has been successfully brought up, and also test that the new process-status `exit` returned on MS-Windows with `taskkill` is set.
10. Introduced integration tests covering jack-in for bb, clojure cli, lein and shadow.

Author: ikappaki

.circleci/config.yml

@@ -15,6 +15,18 @@ commands:
           name: Install unzip
           command: apt-get update && apt-get install unzip
 
+  macos-setup:
+    steps:
+      - checkout
+      - run:
+          name: Install Emacs latest
+          command: |
+            echo "HOMEBREW_NO_AUTO_UPDATE=1" >> $BASH_ENV
+            brew install homebrew/cask/emacs
+      - run:
+          name: Install Eldev
+          command: curl -fsSL https://raw.github.com/doublep/eldev/master/webinstall/circle-eldev > x.sh && source ./x.sh
+
   setup-windows:
     steps:
       - checkout
@@ -74,6 +86,13 @@ jobs:
       - setup
       - test
 
+  test-macos-emacs-latest:
+    macos:
+      xcode: "14.0.0"
+    steps:
+      - macos-setup
+      - test
+
   test-windows-emacs-latest:
     executor: win/default
     steps:
@@ -100,4 +119,5 @@ workflows:
       - test-emacs-28
       - test-emacs-master
       - test-lint
+      - test-macos-emacs-latest
       - test-windows-emacs-latest

.codespellrc

@@ -0,0 +1,2 @@
+[codespell]
+skip = .git,.eldev,logo

.github/workflows/test.yml

@@ -3,26 +3,68 @@ name: CI
 on: [push, pull_request]
 
 jobs:
-  test:
+  integration:
+    # Run integration tests for all OSs and EMACS_VERSIONs.
     runs-on: ${{matrix.os}}
 
     strategy:
       matrix:
-        os: [macos-latest]
-        emacs_version: ['26.3', '27.2', '28.1']
+        os: [macos-latest, ubuntu-latest, windows-latest]
+        emacs_version: ['26.3', '27.2', '28.2']
 
     steps:
     - name: Set up Emacs
+      if: "!startsWith (matrix.os, 'windows')"
       uses: purcell/setup-emacs@master
       with:
         version: ${{matrix.emacs_version}}
 
+    - name: Set up Emacs on Windows
+      if: startsWith (matrix.os, 'windows')
+      uses: jcs090218/setup-emacs-windows@master
+      with:
+        version: ${{matrix.emacs_version}}
+
     - name: Install Eldev
+      if: "!startsWith (matrix.os, 'windows')"
       run: curl -fsSL https://raw.github.com/doublep/eldev/master/webinstall/github-eldev | sh
 
+    - name: Install Eldev on MS-Windows
+      if: startsWith (matrix.os, 'windows')
+      run: |
+        # Remove expired DST Root CA X3 certificate. Workaround
+        # for https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51038
+        # bug on Emacs 27.2.
+        gci cert:\LocalMachine\Root\DAC9024F54D8F6DF94935FB1732638CA6AD77C13
+        gci cert:\LocalMachine\Root\DAC9024F54D8F6DF94935FB1732638CA6AD77C13 | Remove-Item
+
+        curl.exe -fsSL https://raw.github.com/doublep/eldev/master/webinstall/github-eldev.bat | cmd /Q
+
     - name: Check out the source code
       uses: actions/checkout@v2
 
-    - name: Test the project
+    - name: Prepare java
+      uses: actions/setup-java@v3
+      with:
+        distribution: 'temurin'
+        # shadow requires java 11
+        java-version: 11
+
+    - name: Install Clojure Tools
+      # Use SHA until
+      # https://github.com/DeLaGuardo/setup-clojure/issues/78 is
+      # released
+      uses: DeLaGuardo/setup-clojure@1376ded6747c79645e82c856f16375af5f5de307
+      with:
+        bb: '1.0.165'
+        cli: '1.10.3.1013'
+        lein: '2.9.10'
+
+    - uses: actions/setup-node@v3
+      with:
+        node-version: 16
+    - run: npm install [email protected] -g
+
+    - name: Test integration
       run: |
-        eldev -p -dtT test
+        eldev -p -dtTC test --test-type integration

Eldev

@@ -10,12 +10,36 @@
 
 (eldev-add-loading-roots 'test "test/utils")
 
-;; Otherwise `cider-test.el' will be considered a test file.
-(setf eldev-test-fileset "./test/")
-;; This file is _supposed_ to be excluded from automated testing.  Since Eldev
-;; uses everything inside `test/' subdirectory, tell it to leave this file alone
-;; explicitly.
-(setf eldev-standard-excludes `(:or ,eldev-standard-excludes "test/cider-tests--no-auto.el"))
+(defvar cider-test-type 'main)
+(setf eldev-standard-excludes `(:or ,eldev-standard-excludes
+                                    ;; Avoid including files in test "projects".
+                                    (eldev-pcase-exhaustive cider-test-type
+                                                            (`main        "./test/*/")
+                                                            (`integration '("./test/"   "!./test/integration"))
+                                                            (`all         '("./test/*/" "!./test/integration")))
+                                    "test/integration/projects"
+                                    ;; This file is _supposed_ to be excluded
+                                    ;; from automated testing.
+                                    "test/cider-tests--no-auto.el"))
+
+(eldev-defoption cider-test-selection (type)
+  "Select tests to run; type can be `main', `integration' or `all'"
+  :options        (-T --test-type)
+  :for-command    test
+  :value          TYPE
+  :default-value  cider-test-type
+  (unless (memq (intern type) '(main integration all))
+    (signal 'eldev-wrong-option-usage `("unknown test type `%s'" ,type)))
+  (setf cider-test-type (intern type)))
+
+(add-hook 'eldev-test-hook
+          (lambda ()
+            (eldev-verbose "Using cider tests of type `%s'" cider-test-type)))
+(add-hook 'eldev-executing-command-hook
+          (lambda (command)
+            (unless (eq command 'test)
+              ;; So that e.g. byte-compilation works on all tests.
+              (setf cider-test-type 'all))))
 
 ;; CIDER cannot be compiled otherwise.
 (setf eldev-build-load-before-byte-compiling t)

cider-connection.el

@@ -180,6 +180,8 @@ buffer."
                      ;; Sync request will hang if the server is dead.
                      (process-live-p (get-buffer-process nrepl-server-buffer))))
         (nrepl-sync-request:close repl)
+        ;; give a chance to the REPL to respond to the closing of the connection
+        (sleep-for 0.5)
         (delete-process proc)))
     (when-let* ((messages-buffer (and nrepl-log-messages
                                       (nrepl-messages-buffer repl))))

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

@@ -84,9 +84,20 @@ all the details.
 
 If you prefer running all tests outside Emacs that's also an option.
 
-Run all tests with:
+There are two test types, `main` (unit tests) and `integration`. By
+default only main tests are run.
 
- $ eldev test
+Run all unit tests with:
+
+ $ eldev[.bat] test
+
+Run integration tests with:
+
+ $ eldev[.bat] test --test-type integration
+
+or all tests with
+
+ $ eldev[.bat] test --test-type all
 
 NOTE: Tests may not run correctly inside Emacs' `shell-mode` buffers. Running
 them in a terminal is recommended.

nrepl-client.el

@@ -627,12 +627,33 @@ If NO-ERROR is non-nil, show messages instead of throwing an error."
 ;;; Client: Process Handling
 
 (defun nrepl--kill-process (proc)
-  "Kill PROC using the appropriate, os specific way.
-Implement a workaround to clean up an orphaned JVM process left around
-after exiting the REPL on some windows machines."
-  (if (memq system-type '(cygwin windows-nt))
-      (interrupt-process proc)
-    (kill-process proc)))
+  "Attempt to kill PROC tree.
+On MS-Windows, using the standard API is highly likely to leave the child
+processes still running in the background as orphans.  As a workaround, an
+attempt is made to delegate the task to the `taskkill` program, which comes
+with windows since at least Windows XP, and fallback to the Emacs API if it
+can't be found.
+
+It is expected that the `(process-status PROC)` return value after PROC is
+killed is `exit` when `taskkill` is used and `signal` otherwise."
+  (cond
+   ((and (eq system-type 'windows-nt)
+         (process-live-p proc)
+         (executable-find "taskkill"))
+    ;; try to use `taskkill` if available
+    (with-temp-buffer
+      (call-process-shell-command (format "taskkill /PID %s /T /F" (process-id proc))
+                                  nil (buffer-name) )
+      ;; useful for debugging.
+      ;;(message ":PROCESS-KILL-OUPUT %s" (buffer-string))
+      ))
+
+   ((memq system-type '(cygwin windows-nt))
+    ;; fallback, this is considered to work better than `kill-process` on
+    ;; MS-Windows.
+    (interrupt-process proc))
+
+   (t (kill-process proc))))
 
 (defun nrepl-kill-server-buffer (server-buf)
   "Kill SERVER-BUF and its process."
@@ -1090,8 +1111,22 @@ match groups:
 1  for the port, and
 2  for the host (babashka only).")
 
+(defun cider--process-plist-put (proc prop val)
+  "Change value in PROC's plist of PROP to VAL.
+Value is changed using `plist-put`, of which see."
+  (thread-first
+    proc
+    (process-plist)
+    (plist-put prop val)
+    (thread-last (set-process-plist proc))))
+
 (defun nrepl-server-filter (process output)
-  "Process nREPL server output from PROCESS contained in OUTPUT."
+  "Process nREPL server output from PROCESS contained in OUTPUT.
+
+The PROCESS plist is updated as (non-exhaustive list):
+
+:cider--nrepl-server-ready set to t when the server is successfully brought
+up."
   ;; In Windows this can be false:
   (let ((server-buffer (process-buffer process)))
     (when (buffer-live-p server-buffer)
@@ -1117,6 +1152,7 @@ match groups:
             (setq nrepl-endpoint (list :host host
                                        :port port))
             (message "[nREPL] server started on %s" port)
+            (cider--process-plist-put process :cider--nrepl-server-ready t)
             (when nrepl-on-port-callback
               (funcall nrepl-on-port-callback (process-buffer process)))))))))
 
@@ -1129,16 +1165,11 @@ match groups:
 
 (declare-function cider--close-connection "cider-connection")
 (defun nrepl-server-sentinel (process event)
-  "Handle nREPL server PROCESS EVENT."
-  (let* ((server-buffer (process-buffer process))
-         (clients (seq-filter (lambda (b)
-                                (eq (buffer-local-value 'nrepl-server-buffer b)
-                                    server-buffer))
-                              (buffer-list)))
-         (problem (if (and server-buffer (buffer-live-p server-buffer))
-                      (with-current-buffer server-buffer
-                        (buffer-substring (point-min) (point-max)))
-                    "")))
+  "Handle nREPL server PROCESS EVENT.
+On a fatal EVENT, attempt to close any open client connections, and signal
+an `error' if the nREPL PROCESS exited because it couldn't start up."
+  ;; only interested on fatal signals.
+  (when (not (process-live-p process))
     (emacs-bug-46284/when-27.1-windows-nt
      ;; There is a bug in emacs 27.1 (since fixed) that sets all EVENT
      ;; descriptions for signals to "unknown signal". We correct this by
@@ -1151,17 +1182,22 @@ match groups:
          (2 (setq event "Interrupt"))
          ;; SIGKILL==9 emacs nt/inc/ms-w32.h
          (9 (setq event "Killed")))))
-
-    (when server-buffer
-      (kill-buffer server-buffer))
-    (cond
-     ((string-match-p "^killed\\|^interrupt" event)
-      nil)
-     ((string-match-p "^hangup" event)
-      (mapc #'cider--close-connection clients))
-     ;; On Windows, a failed start sends the "finished" event. On Linux it sends
-     ;; "exited abnormally with code 1".
-     (t (error "Could not start nREPL server: %s" problem)))))
+    (let* ((server-buffer (process-buffer process))
+           (clients (seq-filter (lambda (b)
+                                  (eq (buffer-local-value 'nrepl-server-buffer b)
+                                      server-buffer))
+                                (buffer-list))))
+      ;; close any known open client connections
+      (when server-buffer
+        (kill-buffer server-buffer))
+      (mapc #'cider--close-connection clients)
+
+      (if (process-get process :cider--nrepl-server-ready)
+          (message "nREPL server exited.")
+        (let ((problem (when (and server-buffer (buffer-live-p server-buffer))
+                         (with-current-buffer server-buffer
+                           (buffer-substring (point-min) (point-max))))))
+          (error "Could not start nREPL server: %s (%S)" problem (string-trim event)))))))
 
 
 ;;; Messages

test/cider-tests.el

@@ -555,11 +555,12 @@
               "(do (require '[shadow.cljs.devtools.api :as shadow]) (shadow/browser-repl))")))
   (describe "can watch multiple builds"
     (it "watches 2 builds and selects user-defined builds"
-      (setq-local cider-shadow-default-options "client-build")
-      (setq-local cider-shadow-watched-builds '("client-build" "other-build"))
-      (expect (cider-shadow-cljs-init-form)
-              :to-equal
-              "(do (require '[shadow.cljs.devtools.api :as shadow]) (shadow/watch :client-build) (shadow/watch :other-build) (shadow/nrepl-select :client-build))"))))
+      (with-temp-buffer
+        (setq-local cider-shadow-default-options "client-build")
+        (setq-local cider-shadow-watched-builds '("client-build" "other-build"))
+        (expect (cider-shadow-cljs-init-form)
+                :to-equal
+                "(do (require '[shadow.cljs.devtools.api :as shadow]) (shadow/watch :client-build) (shadow/watch :other-build) (shadow/nrepl-select :client-build))")))))
 
 (describe "cider--resolve-project-command"
   (it "if command starts with ./ it resolves relative to clojure-project-dir"