Support rebinding `clojure.test/test-var`

[colin-p-hill]

The context

I am attempting to write a runner-agnostic test fixture that modifies test behavior by wrapping and rebinding clojure.test/test-var, which is a dynamic var. Here is a small example of the basic pattern:

Test code

```clojure (ns rebind-test (:require [clojure.test :as t])) (t/use-fixtures :once (fn [f] (let [base-test-var t/test-var] (binding [t/test-var (fn [v] (println "Hello from the middleware!") (base-test-var v))] (f))))) (t/deftest trivial-test (println "Hello from the test!") (t/is false)) ```

The problem

While this works with Cognitect's runner, the middleware fails to execute with Kaocha.

Cognitect runner output

```text > clj -Sdeps '{:aliases {:test {:extra-paths ["test"] :extra-deps {io.github.cognitect-labs/test-runner {:git/tag "v0.5.1" :git/sha "dfb30dd"}} :main-opts ["-m" "cognitect.test-runner"] :exec-fn cognitect.test-runner.api/test}}}}' Running tests in #{"test"} Testing rebind-test Hello from the middleware! Hello from the test! FAIL in (trivial-test) (rebind_test.clj:14) expected: false actual: false Ran 1 tests containing 1 assertions. 1 failures, 0 errors. Execution error (ExceptionInfo) at cognitect.test-runner.api/test (api.clj:30). Test failures or errors occurred. Full report at: C:\Users\XXXXXX~1\AppData\Local\Temp\clojure-16002871864379326074.edn ```

Kaocha output

```text > clj -Sdeps '{:aliases {:test {:extra-paths ["test"] :extra-deps {lambdaisland/kaocha {:mvn/version "1.77.1236"}} :main-opts ["-m" "kaocha.runner"]}}}}' -M:test --reporter documentation WARNING: Did not load a configuration file and using the defaults. This is fine for experimenting, but for long-term use, we recommend creating a configuration file to avoid changes in behavior between releases. To create a configuration file using the current defaults and configuration file location, create a file named that contains '#kaocha/v1 {}'. --- unit (clojure.test) --------------------------- rebind-test trivial-test FAIL Randomized with --seed 1907191954 FAIL in rebind-test/trivial-test (rebind_test.clj:14) expected: false actual: false ?????? Test output ??????????????????????????????????????????????????????? ? Hello from the test! ?????????????????????????????????????????????????????????????????????????? 1 tests, 1 assertions, 1 failures. ```

The cause

It seems Kaocha defines its own nearly identical function, bypassing the standard one.

clojure.test/test-var

https://github.com/clojure/clojure/blob/527b330045ef35b47a968d80ed3dc4999cfa2623/src/clj/clojure/test.clj#L708-L721 ```clojure (defn test-var "If v has a function in its :test metadata, calls that function, with *testing-vars* bound to (conj *testing-vars* v)." {:dynamic true, :added "1.1"} [v] (when-let [t (:test (meta v))] (binding [*testing-vars* (conj *testing-vars* v)] (do-report {:type :begin-test-var, :var v}) (inc-report-counter :test) (try (t) (catch Throwable e (do-report {:type :error, :message "Uncaught exception, not in assertion." :expected nil, :actual e}))) (do-report {:type :end-test-var, :var v})))) ```

kaocha.type.var/test-var

https://github.com/lambdaisland/kaocha/blob/52f42b78037fee79a1b8b107394b55b1f6a36b3e/src/kaocha/type/var.clj#L20-L28

A look through the revision history suggests that this is a case of convergent evolution. Until recently, Kaocha's function was a fair bit more complicated, but a refactor a few months ago extracted this near-duplicate logic.

Would it be possible to turn this into a binding of clojure.test's var, so that extensions which use the standard API work as expected?

[plexus]

This would be a breaking change for us. Are there other projects that use this mechanism? What is your tool trying to achieve?

The main difference is that we separate the var from its test earlier in the process, so that plugins and hooks can access it, and possibly wrap it or replace it. Which I'm also not sure if it's actually widely used, but it's a mechanism I'd somewhat prefer to keep available. Kaocha's test-var also doesn't increment the :test counter, since we rely on testing the number of tests we see in the result, not on this side-effectful tracking, but I think this might not be that problematic, I think it would just be ignored.

I do see the appeal of being a bit more vanilla in this case, but it does mean making kaocha's own extension mechanisms less powerful, so I think we need to look at what's out there to help make a decision. We are at the point where we are very reluctant to break Kaocha, including its extension API. If we do make this change, and then it turns out that someone was actually relying on Kaocha's behavior, then we would almost certainly revert the change.

[colin-p-hill]

Are there other projects that use this mechanism?

Unsure. I wasn't aware of it until I was poking around clojure.test for a way to solve my particular problem, and noticed that this function was dynamic.

What is your tool trying to achieve?

I'm not 100% sure I remember right (gosh, this was only a month and a half ago? feels like twice that), but I think I was writing a test fixture to implement test timeouts. I was hacking at the generators behind some generative tests, and when I got them wrong, the --watch process would hang while it did an extremely inefficient search. I preferred to implement this as a test fixture to minimize coupling to a specific runner, to leave room for particular namespaces/tests to opt in and configure their own timeouts, and to keep the test file relatively self-contained and self-explanatory.

This would be a breaking change for us.

I haven't taken the time to do a deep dive into Koacha's architecture to reason it through fully, so I don't know offhand what the breakage would be, but if you're referring to the difference in signature, that could be worked around. Or at least, I thought I saw a workaround when I wrote this issue. Something like this:

- (defn test-var [test the-var]
+ (defn test-var [the-var] ;; Other signature can be preserved with another arglist, if needed.
    (binding [t/*testing-vars* (conj t/*testing-vars* the-var)]
      (t/do-report {:type :begin-test-var, :var the-var})
      (try
-       (test)
+       ((:test (meta the-var))) ;; Could use another key if clobbering would be an issue
        (catch clojure.lang.ExceptionInfo e
          (when-not (:kaocha/fail-fast (ex-data e))
            (report/report-exception e)))
        (catch Throwable e (report/report-exception e)))))

  (defmethod testable/-run :kaocha.type/var [{test    :kaocha.var/test
                                              wrap    :kaocha.testable/wrap
                                              the-var :kaocha.var/var
                                              meta'   :kaocha.testable/meta
                                              :as     testable} test-plan]
    (type/with-report-counters
+     (binding [t/test-var test-var]
-       (let [wrapped-test (fn [] (test-var test the-var))
+       (let [wrapped-test (fn [] (t/test-var (vary-meta the-var assoc :test test)))
              wrapped-test (reduce #(%2 %1) wrapped-test wrap)]          
          (wrapped-test)
          (let [{::result/keys [pass error fail pending] :as result} (type/report-count)]
            (when (= pass error fail pending 0)
              (binding [testable/*fail-fast?* false
                        testable/*test-location* {:file (:file meta') :line (:line meta')}]
                (t/do-report {:type ::zero-assertions}))))
          (t/do-report {:type :end-test-var, :var the-var})
-         (merge testable {:kaocha.result/count 1} (type/report-count)))))
+         (merge testable {:kaocha.result/count 1} (type/report-count))))))

Not the prettiest thing in the world (and I haven't tested it), but looks like a workable direction from what I can see, and it would mean being able to have your cake and eat it too. What do you think?

[plexus]

It sounds like this is academic at this point, since you are no longer working on something that requires this? Unless there's a strong driving use case I don't think we want to make any changes here.

Unfortunately vars don't implement vary-meta, only alter-meta! which makes sense since they are identities rather than values.

[colin-p-hill]

It sounds like this is academic at this point, since you are no longer working on something that requires this?

Well, yes and no. I would still like to implement this, but I gave up because it was not possible with Kaocha. Truthfully, this was one of the papercuts that prompted me to put migrating away from Kaocha in my backlog. If this changed, it would cross something off my list of reasons to move. So...yes, it's academic, in the sense that I intend to work around it by not using Kaocha at all, but I figured that was all the more reason to put it on your radar as a feature someone wanted at some point.

Unfortunately vars don't implement vary-meta, only alter-meta!

Dagnabbit. In that case, yeah, I don't see any clear way to have it both ways. At least, not without calling alter-meta! inside the runner code, which I probably wouldn't want to do if our positions were reversed.

Unless there's a strong driving use case I don't think we want to make any changes here.

Fair enough, I completely understand.

[plexus]

Note that Kaocha provides ample extension mechanisms, and implementing a test timeout would be fairly straightforward using a plugin or hooks. I think this is actually a common enough use case that it would be nice to have a kaocha.plugin.timeout in the core distribution.