`deftest` generation at the REPL pollutes the namespace

[ggeoffrey]

clojure.test/deftest expends to def. It is named.

When tests expands to clojure.test/deftest, a unique name is generated (based on the line number). If you move a tests block and re-evaluate it, the generated test get a new identity. It doesn’t overwrite its previous version, polluting the namespace.

All tests (current and previous versions) will be executed by clojure.test/run-tests.

Find a way to provide a stable identity to deftest, or to generate tests differently.

(t/deftest test-identity-name …)

Thanks to @seancorfield on Clojurians Slack for reporting the issue.

[seancorfield]

I ran into this with Expectations and it was a big driver for me to create the clojure.test-compatible version of Expectations, where the main difference was tests had stable names.

The use case that matters for auto-generated names is really just the interactive-in-an-editor environment so it only matters when you have the JVM option in place to generate tests and you are running tests from your editor, although I do wonder how RCF interacts with auto-test-running watchers? (a workflow I really don't like)

My initial thoughts on addressing this would be to provide a new version of tests that requires a symbol to name the tests -- perhaps named-tests <symbol> or some initial syntax in tests to provide a name -- perhaps (tests :as <symbol> ...) -- and then provide a new option that only generates deftest from the named form of tests (and not from the unnamed tests).

That would allow the current behavior -- which is good for external test runs and therefore in CI -- to continue unchanged, while providing a new set of behaviors for folks wanting to use a combination of simple inline evaluations (tests today) and actual, generated named tests that remain stable during editing. I would definitely want both types of tests to be available for dev work: the unnamed forms that evaluate when you load the file and when you evaluate the form directly; and the named forms that also provide a stable named test as well as being evaluated at load and directly.

[dustingetz]

I like the issue - the test label combined with the current namespace could be enforced as unique but what if you change the label? What would clojure.test do in this circumstance? This is a clojure layer issue right?

[seancorfield]

If you change the name of a deftest and eval it, you still have the old named test as well as the new one. So that's an "existing problem" (with the same solutions that people already use).

[seancorfield]

I had a look at how RCF works and providing some sort of extension to tests to signal a named test is pretty straightforward -- I have a fork of RCF with (tests :as some-name ...) for specifically-named tests -- but figuring out a good UX for selecting this "third" behavior, where only named tests get generated but unnamed tests do not, that would work across both clj and cljs, is hard...

From the clj side, my reaction would be just to have hyperfiddle.rcf.generate-tests just become tri-valued: false, true, named but I don't know how that would play out for cljs based on looking at the reader conditions around that?

[seancorfield]

See https://github.com/seancorfield/rcf/commit/3074171fd509f687c5b228db59cacabd3d40e173

[ggeoffrey]

Thank you for the example, it makes sense.

I slept over it and believe we should not retrofit clojure.test design into RCF. clojure.test tests are named by design. Rich Comment Forms are not.

Here is a different POV, separating concerns:

(defmacro generate-tests!
  "Generate a `clojure.test/deftest` running RCF in `*ns*`.

  Arities:
  0: produces a `deftest` running all RCF tests in `*ns*`.
  1: produces a named `deftest` running all RCF tests in `*ns*`.
  n: produces a named `deftest` running listed RCF tests.
     Use `(rcf/tests {:name my-test-name} …)` to produce a named RCF test.
  "
  ([])
  ([name])
  ([name & test-names])
  )

(defmacro generate-all-tests!
  "Generate a `clojure.test/deftest` in all namespaces, running RCF tests in these namespace.
  List them or provide a regexp to filter which namespaces should be impacted.
  Unless specified otherwise, will not overload calls to `generate-tests!`,
  allowing for fine-grained namespace-level configuration.

  Examples:
  (generate-all-tests!) ; one `deftest` for each ns containing RCFs
  (generate-all-tests! true) ; same but overload local `generate-tests!` calls
  (generate-all-tests! #\"my-project.*\") ; select namespaces
  (generate-all-tests! true my-project.core my-project.lib)  ; overload in selected namespaces
  "
  [,,,])

I’m not yet convinced by the {:name my-test-name} syntax.

[dustingetz]

Would this work? (ns dustin.foo) (tests "hello world" 1 := 1) test key is "dustin.foo hello world"

Integrating clojure.test at the namespace level is an interesting insight, @seancorfield would that suit your workflow

[seancorfield]

@dustingetz That's an interesting -- and mostly compatible -- approach, using the first string as the symbolic name of the test, if present. I suspect that folks would be more likely to run into naming conflicts per your question above and my response when using strings, rather than an explicit name: using an explicit symbol makes you think about the name a bit more. I would be concerned about the possibility of generating "broken" code, by converting an arbitrary string to a symbol and using it as a function name (a test name) but I'd have to do some experiments to see how real that concern is, e.g., would (tests "a/b-testing" ...) produce a valid definition? (the namespace is unnecessary since tests are local to a namespace already so you do not need to worry about global uniqueness)

@ggeoffrey I think that there's great value in having RCFs executed as tests during CI and that one-shot generation of synthetic test names is going to continue to work just fine in that scenario -- and having them actually contribute to the number of tests passed/failed, rather than just executing at load time is much better for whatever test runner folks are using in CI.

I think my workflow is probably an edge case with RCF, wanting both the "regular" REPL-based workflow (which is easy to enable in user.clj or whatever someone uses when starting their REPL) and generated tests to persist for those RCF examples when running tests from the REPL/editor. I think it's a good workflow, but I don't think it's very common 😄 I don't think that adding the complexity of that multi-arity generate* call and its supporting logic is worth it for the use case I'm after.

I'm also not convinced by the {:name my-test-name} syntax since (tests {:some hash-map} ...) could well be the start of a valid RCF and having that behavior determined by some runtime flag is potentially confusing and/or brittle -- I'd be more inclined to try Dustin's suggestion or to have an explicit syntax that could not be mistaken for part of a "regular" RCF, hence my :as some-name suggestion.

[seancorfield]

@dustingetz I just confirmed that trying to deftest based on an arbitrary string can fail:

• (tests "a/b-testing" ...) produces java.lang.RuntimeException: "Can't refer to qualified var that doesn't exist"
• (tests "a... b-testing" ...) produces java.lang.ClassNotFoundException: "a/// b-testing"

[seancorfield]

I updated my fork to suppress generation of unnamed tests when inline evaluation is enabled (so you only get named tests in the REPL): https://github.com/seancorfield/rcf/commit/33eb8358fed1fc2b7a9a8df650d74b7b68cc5aad

That is working pretty well for me. In CI, it's just regular test generation (no change from your version). If you enable it without generate tests, you get the same inline evaluation (like your version). If you enable it and also ask for test generation, you won't get deftest except for (tests :as test-name ...) forms so there's no "namespace pollution" and you get stable test names (only).

[seancorfield]

Any further thoughts on this? Would you like a PR based on my fork, with the behavior described above? What additional supporting code, tests, docs would you require in such a PR?

[dustingetz]

@seancorfield we're wrapping up a rewrite using tools.anaylzer to get correct term rewriting, we will revisit this later. We need to wrap this up before moving on. Thanks for your interest