Introduce integration tests

.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

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### New features
 
+- [#3278](https://github.com/clojure-emacs/cider/pull/3278) Introduce integration tests, which also fix a long standing issue with orphaned process on MS-Windows by contracting `taskkill`, if available, to properly kill the nREPL server process tree.
 - [#3249](https://github.com/clojure-emacs/cider/pull/3249): Add support for Clojure Spec 2.
 - [#3247](https://github.com/clojure-emacs/cider/pull/3247) Add the `cider-stacktrace-analyze-at-point` and `cider-stacktrace-analyze-in-region` commands to view printed exceptions in the stacktrace inspector.
 

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"