Polish up parallelization: clearer config, docs, vestigial code

Clean up the changes around parallelization, make opting-in from test types
explicit, allow config on testable and metadata level, simplify future handling.

Author: plexus

doc/03_configuration.md

@@ -29,7 +29,8 @@ Here's an example test configuration with a single test suite:
  :kaocha.plugin.randomize/seed       950716166
  :kaocha.plugin.randomize/randomize? true
  :kaocha.plugin.profiling/count      3
- :kaocha.plugin.profiling/profiling? true}
+ :kaocha.plugin.profiling/profiling? true
+ :kaocha/parallize?                  false}
 ```
 
 Writing a full test configuration by hand is tedious, which is why in

doc/11_parallelization.md

@@ -0,0 +1,85 @@
+# 11. Parallelization
+
+Parallelization is an optional Kaocha feature, where it distributes your test
+workload across multiple threads, to make better use of multiple CPU cores.
+
+This is still a relatively new feature, one that has a chance of interfering in
+various ways with plugins, custom hooks, or the particulars of test setups that
+people have "in the wild". We very much welcome feedback and improvements.
+Please be mindful and respectful of the maintainers though and report issues
+clearly, preferably with a link to a git repository containing a minimal
+reproduction of the issue.
+
+## Configuration and high-level behavior
+
+You can enable parallelization either via the `--parallelize` command line flag,
+or by setting `:parallelize? true` in your `tests.edn`. This is assuming that
+you are using the `#kaocha/v1` reader literal to provide normalization. The
+canonical configuration key is `:kaocha/parallelize?`.
+
+Kaocha looks at your tests as a hierarchy, at the top level there are your test
+suites (e.g. unit vs intergration, or clj vs cljs). These contain groups of
+tests (their children), e.g. one for each namespace, and these in turn contain
+multiple tests, e.g. one for each test var.
+
+Setting `:parallelize true?` at the top-level configuration, or using the
+command line flag, will run any suites you have in parallel, as well making
+parallelization the default for any type of testable that has children. So say
+for instance you have a suite of type `clojure.test`, then multiple test
+namespaces will be run in parallel, and individual test vars within those
+namespaces will also be started in parallel.
+
+Test type implementations need to opt-in to parallelization. For instance,
+Clojure is multi-threaded, but ClojureScript (running on a JavaScript runtime)
+is not, so thre is little benefit in trying to parallelize ClojureScript tests.
+So even with parallelization on, `kaocha.cljs` or `kaocha.cljs2` tests will
+still run in series.
+
+## Fine-grained opting in or out
+
+Using the command line flag or setting `:parallelize? true` at the top-level of
+tests.edn will cause any testable that is parallelizable to be run in parallel.
+If you want more fine-grained control you can configure specific test suites to
+be parallelized, or set metadata on namespaces to opt in or out of
+parallelization.
+
+```clj
+#kaocha/v1
+{:tests [{:id :unit, :parallelize? true}])
+```
+
+This will cause all namespaces in the unit test suite to be run in parallel, but
+since the default (top-level config) is not set, vars within those namespaces
+will not run in parallel. But you can again opt-in at that specific level,
+through metadata.
+
+```clj
+(ns ^{:kaocha/parallelize? true} my-test
+ (:require [clojure.test :as t]))
+  
+...
+```
+
+Conversely you can opt-out of parallelization on the test suite or test
+namespace level by setting `:kaocha/parallelize? false`.
+
+## Caveats
+
+When you start running your tests in parallel you will likely notice one or two
+things. The first is that your output looks all messed up. Before you might see
+something like `[(....)(.......)][(.......)]`, whereas now it looks more like
+`[[(..(..).....(..)...]....)]`. This will be even more pronounced if you are for
+instance using the documentation reporter. Output from multiple tests gets
+interleaved, causing a garbled mess.
+
+The default dots reporter is probably the most usable reporter right now.
+
+The second thing you might notice is that you are getting failures where before
+you got none. This likely indicates that your tests themselves are not thread
+safe. They may for instance be dealing with shared mutable state.
+
+You will have to examine your code carefully. Starting out with a more piecemeal
+opting in might be helpful to narrow things down.
+
+It is also possible that you encounters failures caused by Kaocha itself. In
+that case please report them on our issue tracker.

src/kaocha/api.clj

@@ -123,11 +123,6 @@
                                       " To investigate, check the :test-paths and :ns-patterns keys in tests.edn.")))
                   (throw+ {:kaocha/early-exit 0}))
 
-                (when (:parallel config)
-                  (output/warn (str "Parallelization enabled. This feature is in "
-                                    "beta If you encounter errors, try "
-                                    "running with the feature disabled and "
-                                    "consider filing an issue.")))
                 (when (find-ns 'matcher-combinators.core)
                   (require 'kaocha.matcher-combinators))
 
@@ -142,8 +137,7 @@
                                       ;; don't know where in the process we've
                                       ;; been interrupted, output capturing may
                                       ;; still be in effect.
-                                      (System/setOut
-                                       orig-out)
+                                      (System/setOut orig-out)
                                       (System/setErr
                                        orig-err)
                                       (binding [history/*history* history]
@@ -160,13 +154,13 @@
                                             on-exit)
                         (let [test-plan (plugin/run-hook :kaocha.hooks/pre-run test-plan)]
                           (binding [testable/*test-plan* test-plan]
-                            (let [test-plan-tests (:kaocha.test-plan/tests test-plan)
-                                  result-tests    (testable/run-testables test-plan-tests test-plan)
-                                  result          (plugin/run-hook :kaocha.hooks/post-run
-                                                                   (-> test-plan
-                                                                       (dissoc :kaocha.test-plan/tests)
-                                                                       (assoc :kaocha.result/tests result-tests)))]
-                              (assert (= (count test-plan-tests) (count (:kaocha.result/tests result))))
+                            (let [result-tests (testable/run-testables-parent test-plan test-plan)
+                                  result       (plugin/run-hook :kaocha.hooks/post-run
+                                                          (-> test-plan
+                                                              (dissoc :kaocha.test-plan/tests)
+                                                              (assoc :kaocha.result/tests result-tests)))]
+                              (assert (= (count (:kaocha.test-plan/tests test-plan))
+                                         (count (:kaocha.result/tests result))))
                               (-> result
                                   result/testable-totals
                                   result/totals->clojure-test-summary

src/kaocha/config.clj

@@ -55,7 +55,8 @@
               (rename-key :skip :kaocha.filter/skip)
               (rename-key :focus :kaocha.filter/focus)
               (rename-key :skip-meta :kaocha.filter/skip-meta)
-              (rename-key :focus-meta :kaocha.filter/focus-meta))]
+              (rename-key :focus-meta :kaocha.filter/focus-meta)
+              (rename-key :parallelize? :kaocha/parallelize?))]
     (as-> m $
       (merge-config (first (:kaocha/tests (default-config))) $)
       (merge {:kaocha.testable/desc (str (name (:kaocha.testable/id $))
@@ -82,7 +83,8 @@
                 randomize?
                 capture-output?
                 watch?
-                bindings]} config
+                bindings
+                parallelize?]} config
         tests (some->> tests (mapv normalize-test-suite))]
     (cond-> {}
       tests                   (assoc :kaocha/tests (vary-meta tests assoc :replace true))
@@ -95,6 +97,7 @@
       (some? watch?)          (assoc :kaocha/watch? watch?)
       (some? randomize?)      (assoc :kaocha.plugin.randomize/randomize? randomize?)
       (some? capture-output?) (assoc :kaocha.plugin.capture-output/capture-output? capture-output?)
+      (some? parallelize?)    (assoc :kaocha/parallelize? parallelize?)
       :->                     (merge (dissoc config :tests :plugins :reporter :color? :fail-fast? :watch? :randomize?)))))
 
 (defmethod aero/reader 'kaocha [_opts _tag value]
@@ -195,17 +198,16 @@
      config
      (read-config nil opts))))
 
-
 (defn apply-cli-opts [config options]
   (cond-> config
-    (some? (:fail-fast options))  (assoc :kaocha/fail-fast? (:fail-fast options))
-    (:reporter options)           (assoc :kaocha/reporter (:reporter options))
-    (:watch options)              (assoc :kaocha/watch? (:watch options))
-    (some? (:color options))      (assoc :kaocha/color? (:color options))
-    (some? (:diff-style options)) (assoc :kaocha/diff-style (:diff-style options))
-    (:plugin options)             (update :kaocha/plugins #(distinct (concat % (:plugin options))))
-    (some? (:parallel options))   (assoc :parallel (:parallel options))
-    true                          (assoc :kaocha/cli-options options)))
+    (some? (:fail-fast options))   (assoc :kaocha/fail-fast? (:fail-fast options))
+    (:reporter options)            (assoc :kaocha/reporter (:reporter options))
+    (:watch options)               (assoc :kaocha/watch? (:watch options))
+    (some? (:color options))       (assoc :kaocha/color? (:color options))
+    (some? (:diff-style options))  (assoc :kaocha/diff-style (:diff-style options))
+    (:plugin options)              (update :kaocha/plugins #(distinct (concat % (:plugin options))))
+    (some? (:parallelize options)) (assoc :kaocha/parallelize? (:parallelize options))
+    true                           (assoc :kaocha/cli-options options)))
 
 (defn apply-cli-args [config args]
   (if (seq args)

src/kaocha/runner.clj

@@ -34,7 +34,7 @@
    [nil  "--[no-]fail-fast"     "Stop testing after the first failure."]
    [nil  "--[no-]color"         "Enable/disable ANSI color codes in output. Defaults to true."]
    [nil  "--[no-]watch"         "Watch filesystem for changes and re-run tests."]
-   [nil  "--[no-]parallel"           "Run tests in parallel. Warning: This feature is beta."]
+   [nil  "--[no-]parallelize"   "Run tests in parallel."]
    [nil  "--reporter SYMBOL"    "Change the test reporter, can be specified multiple times."
     :parse-fn (fn [s]
                 (let [sym (symbol s)]

src/kaocha/test_suite.clj

@@ -4,10 +4,10 @@
 
 (defn run [testable test-plan]
   (t/do-report {:type :begin-test-suite})
-  (let [results (testable/run-testables (:kaocha.test-plan/tests testable) test-plan) 
+  (let [results (testable/run-testables-parent testable test-plan)
         testable (-> testable
                      (dissoc :kaocha.test-plan/tests)
                      (assoc :kaocha.result/tests results))]
     (t/do-report {:type :end-test-suite
                   :kaocha/testable testable})
-   testable))
+    testable))

src/kaocha/testable.clj

@@ -49,22 +49,6 @@
       (try-require (symbol (namespace type))))
     (try-require (symbol (name type)))))
 
-(defn- retry-assert-spec [type testable n]
-  (let [result (try (assert-spec type testable) (catch Exception _e false))]
-    (if (or result (<= n 1)) result
-        (retry-assert-spec type testable (dec n))) ;otherwise, retry
-))
-
-(defn deref-recur [testables]
-  (cond (future? testables) (deref testables)
-        (vector? testables) (doall (mapv deref-recur testables))
-        (seq? testables) (deref-recur (into [] (doall testables)))
-        (contains? testables :kaocha.test-plan/tests)
-        (update testables :kaocha.test-plan/tests deref-recur)
-        (contains? testables :kaocha.result/tests)
-        (update testables :kaocha.result/tests deref-recur)
-        :else testables))
-
 (defn- load-type+validate
   "Try to load a testable type, and validate it both to be a valid generic testable, and a valid instance given the type.
 
@@ -162,9 +146,9 @@
         result))))
 
 (spec/fdef run
-        :args (spec/cat :testable :kaocha.test-plan/testable
-                     :test-plan :kaocha/test-plan)
-        :ret :kaocha.result/testable)
+  :args (spec/cat :testable :kaocha.test-plan/testable
+                  :test-plan :kaocha/test-plan)
+  :ret :kaocha.result/testable)
 
 (defn load-testables
   "Load a collection of testables, returning a test-plan collection"
@@ -240,18 +224,15 @@
 
 (defn try-run-testable [test test-plan n]
   (let [result (try (run-testable test test-plan) (catch Exception _e false))]
-    (if (or result (> n 1)) result ;success or last try, return
-        (try-run-testable test test-plan (dec n))) ;otherwise retry
-))
-
+    (if (or result (> n 1))
+      ;; success or last try, return
+      result
+      ;; otherwise retry
+      (try-run-testable test test-plan (dec n)))))
 
 (defn run-testables-serial
   "Run a collection of testables, returning a result collection."
   [testables test-plan]
-  (doall testables)
-  #_(print "run-testables got a collection of size" (count testables)
-           " the first of which is "
-           (:kaocha.testable/type (first testables)))
   (let [load-error? (some ::load-error testables)]
     (loop [result  []
            [test & testables] testables]
@@ -261,7 +242,7 @@
                      (assoc ::skip true))
               r (run-testable test test-plan)]
           (if (or (and *fail-fast?* (result/failed? r)) (::skip-remaining? r))
-            (reduce into result [[r] testables])
+            (into (conj result r) testables)
             (recur (conj result r) testables)))
         result))))
 
@@ -272,41 +253,36 @@
      :group-name (.getName (.getThreadGroup thread))}))
 
 (defn run-testables-parallel
-  "Run a collection of testables, returning a result collection."
+  "Run a collection of testables in parallel, returning a result collection."
   [testables test-plan]
-  (doall testables)
   (let [load-error? (some ::load-error testables)
-        types (set (:parallel-children-exclude *config*))
-        suites  (:parallel-suites-exclude *config*)
-        futures (map #(do
+        futures (doall
+                 (map (fn [t]
                         (future
-                          (binding [*config*
-                                    (cond-> *config*
-                                      (contains? types (:kaocha.testable/type %)) (dissoc :parallel)
-                                      (and (hierarchy/suite? %) (contains? suites (:kaocha.testable/desc %))) (dissoc :parallel)
-                                      true (update :levels (fn [x] (if (nil? x) 1 (inc x)))))]
-                            (run-testable (assoc % ::thread (current-thread-info) ) test-plan))))
-                     testables)]
-    (comment (loop [result [] ;(ArrayBlockingQueue. 1024)
-                    [test & testables] testables]
-               (if test
-                 (let [test (cond-> test
-                              (and load-error? (not (::load-error test)))
-                              (assoc ::skip true))
-                       r (run-testable test test-plan)]
-                   (if (or (and *fail-fast?* (result/failed? r)) (::skip-remaining? r))
-                     ;(reduce put-return result [[r] testables])
-                     (reduce into result [[r] testables])
-                     ;(recur (doto result (.put r)) testables)
-                     (recur (conj result r) testables)))
-                 result)))
-    (deref-recur futures)))
+                          (run-testable (assoc t ::thread-info (current-thread-info)) test-plan)))
+                      testables))]
+    (doall (map deref futures))))
 
 (defn run-testables
+  "Original run-testables, left for backwards compatibility, and still usable for
+  test types that don't want to opt-in to parallelization. Generally
+  implementations should move to [[run-testables-parent]]."
   [testables test-plan]
-  (if (:parallel *config*)
-    (doall (run-testables-parallel testables test-plan))
-    (run-testables-serial testables test-plan)))
+  (run-testables-serial testables test-plan))
+
+(defn run-testables-parent
+  "Test type implementations should call this in their [[-run]] method, rather
+  than [[run-testables]], so we can inspect the parent and parent metadata to
+  decide if the children should get parallelized."
+  [parent test-plan]
+  (let [testables (:kaocha.test-plan/tests parent)]
+    (if (or (true? (:kaocha/parallelize? (::meta parent)))               ; explicit opt-in via metadata
+            (and (:kaocha/parallelize? test-plan)                        ; enable parallelization in top-level config
+                 (or (::parallelizable? parent)                          ; test type has opted in, children are considered parallelizable
+                     (:kaocha/parallelize? parent))                      ; or we're at the top level, suites are parallelizable. Can also be used as an explicit override/opt-in
+                 (not (false? (:kaocha/parallelize? (::meta parent)))))) ; explicit opt-out via metadata
+      (run-testables-parallel testables test-plan)
+      (run-testables-serial testables test-plan))))
 
 (defn test-seq [testable]
   (cond->> (mapcat test-seq (remove ::skip (or (:kaocha/tests testable)
@@ -319,12 +295,12 @@
 
 (defn test-seq-with-skipped
   [testable]
- "Create a seq of all tests, including any skipped tests.
+  "Create a seq of all tests, including any skipped tests.
 
  Typically you want to look at `test-seq` instead."
   (cond->> (mapcat test-seq (or (:kaocha/tests testable)
-                                               (:kaocha.test-plan/tests testable)
-                                               (:kaocha.result/tests testable)))
+                                (:kaocha.test-plan/tests testable)
+                                (:kaocha.result/tests testable)))
     ;; When calling test-seq on the top level test-plan/result, don't include
     ;; the outer map. When running on an actual testable, do include it.
     (:kaocha.testable/id testable)

src/kaocha/type/clojure/test.clj

@@ -11,6 +11,7 @@
 
 (defmethod testable/-load :kaocha.type/clojure.test [testable]
   (-> testable
+      (assoc :kaocha.testable/parallelizable? true)
       (load/load-test-namespaces type.ns/->testable)
       (testable/add-desc "clojure.test")))
 

src/kaocha/type/ns.clj

@@ -8,16 +8,19 @@
             [kaocha.type :as type]))
 
 (defn ->testable [ns-name]
-  {:kaocha.testable/type :kaocha.type/ns
-   :kaocha.testable/id   (keyword (str ns-name))
-   :kaocha.testable/desc (str ns-name)
-   :kaocha.ns/name       ns-name})
+  {:kaocha.testable/type            :kaocha.type/ns
+   :kaocha.testable/id              (keyword (str ns-name))
+   :kaocha.testable/desc            (str ns-name)
+   :kaocha.testable/parallelizable? true
+   :kaocha.ns/name                  ns-name})
 
 (defn run-tests [testable test-plan fixture-fn]
   ;; It's not guaranteed the the fixture-fn returns the result of calling the
   ;; tests function, so we need to put it in a box for reference.
-  (let [result (atom (:kaocha.test-plan/tests testable))]
-    (fixture-fn #(swap! result testable/run-testables test-plan))
+  (let [result (promise)]
+    (fixture-fn
+     (fn []
+       (deliver result (testable/run-testables-parent testable test-plan))))
     @result))
 
 (defmethod testable/-load :kaocha.type/ns [testable]