cider-test-run-ns-tests-with-filters - includes/excludes don't work

[jumarko]

Summary

Functioncider-test-run-ns-tests-with-filters doesn't work as expected: includes/excludes aren't taken into account.

The problem is probably a typo: "includes" ("excludes") instead of "include" ("exclude").

• https://github.com/clojure-emacs/cider/blob/master/cider-test.el#L656
• https://github.com/clojure-emacs/cider-nrepl/blob/master/src/cider/nrepl/middleware/test.clj#L296
• https://github.com/clojure-emacs/orchard/blob/master/src/orchard/query.clj#L47

Expected behavior

The specified includes/excludes should be applied and only matching tests should be run.

Actual behavior

It always runs all the tests.

Steps to reproduce the problem

Just run cider-test-run-ns-tests-with-filters in a namespace with some "integration tests"

(deftest unit-test
  (testing "unit"
    (is (= 1 1))))

(deftest ^:integration integration-test
  (testing "integration"
    (is (= 42 42))))

And try to run the tests via cider-test-run-ns-tests-with-filters using empty "includes" and :integration as "excludes".
Cider will always run all the tests not excluding anything

Environment & Version information

CIDER version information

Include here the version string displayed when
CIDER's REPL is launched. Here's an example:

;; CIDER 0.18.0snapshot (package: 20180531.823), nREPL 0.2.13
;; Clojure 1.9.0, Java 10

Emacs version

25.3.1

Operating system

Mac OS High Sierra 10.13.5

[jumarko]

@bbatsov I've just tried the latest build (package: 20180628.628) and it seriously breaks the main cider-test functionality - I cannot run any tests anymore:

Exception in thread "nREPL-worker-0" 
clojure.lang.ArityException: Wrong number of args (0) passed to: core/some-fn
	at clojure.lang.AFn.throwArity(AFn.java:429)
	at clojure.lang.RestFn.invoke(RestFn.java:399)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.RestFn.applyTo(RestFn.java:132)
	at clojure.core$apply.invokeStatic(core.clj:657)
	at clojure.core$apply.invoke(core.clj:652)
	at mranderson048.orchard.v0v3v0_20180519v074938_6.orchard.query$vars$fn__11037.invoke(query.clj:85)
	at clojure.core$filter$fn__5692.invoke(core.clj:2813)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:528)
	at clojure.core$seq__5202.invokeStatic(core.clj:137)
	at clojure.core$filter$fn__5692.invoke(core.clj:2801)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:528)
	at clojure.core$seq__5202.invokeStatic(core.clj:137)
	at clojure.core.protocols$seq_reduce.invokeStatic(protocols.clj:24)
	at clojure.core.protocols$fn__7922.invokeStatic(protocols.clj:75)
	at clojure.core.protocols$fn__7922.invoke(protocols.clj:75)
	at clojure.core.protocols$fn__7868$G__7863__7881.invoke(protocols.clj:13)
	at clojure.core$reduce.invokeStatic(core.clj:6763)
	at clojure.core$group_by.invokeStatic(core.clj:7081)
	at clojure.core$group_by.invoke(core.clj:7081)
	at cider.nrepl.middleware.test$test_var_query.invokeStatic(test.clj:232)
	at cider.nrepl.middleware.test$test_var_query.invoke(test.clj:228)
	at cider.nrepl.middleware.test$handle_test_var_query_op$fn__11168$fn__11169$fn__11170.invoke(test.clj:280)
	at cider.nrepl.middleware.test$handle_test_var_query_op$fn__11168$fn__11169.invoke(test.clj:279)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.core$apply.invokeStatic(core.clj:657)
	at clojure.core$with_bindings_STAR_.invokeStatic(core.clj:1965)
	at clojure.core$with_bindings_STAR_.doInvoke(core.clj:1965)
	at clojure.lang.RestFn.invoke(RestFn.java:425)
	at cider.nrepl.middleware.test$handle_test_var_query_op$fn__11168.invoke(test.clj:277)
	at clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__1179.invoke(interruptible_eval.clj:190)
	at clojure.lang.AFn.run(AFn.java:22)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1135)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:844)

That single "typo" isn't probably the whole story.

[bbatsov]

Seems there's also some mismatch between the params passed by cider-nrepl to orchard (where this filtering actually happens). I'm extremely busy today, so I'll take a look either in the evening or tomorrow. Maybe @SevereOverfl0w will be able to check what's going wrong in the mean time.

[SevereOverfl0w]

I think I'm being stupid, where's the typo (did master change to fix it?).

What's changed in cider.el? The ns query I'm expecting here would be:

{:ns-query {:exactly [(find-ns 'this.ns)]}
 :exclude-meta-key [:integration]}

I'm not an emacs user, so I'm a bit stuck on how to run/debug this, @jumarko are you comfortable using some strategy to debug cider-nrepl (Can you debug cider with cider maybe?).

[bbatsov]

@SevereOverfl0w It was fixed on master. Before the fix here https://github.com/clojure-emacs/cider/blob/master/cider-test.el#L656 Emacs was passing to the middleware includes and excludes, instead of include and exclude. I'm assuming the problem is that an empty vector is probably not handled properly in the query functionality (currently Emacs will send inlude/exclude even if they're empty on the client side).

[bbatsov]

(that's just a guess, because before I fixed the middleware call we were just not sending anything meaningful to the middleware and now we send empty include/exclude most of the time).

[SevereOverfl0w]

clojure.lang.ArityException: Wrong number of args (0) passed to: core/some-fn

This makes sense if there's an empty list being passed. This goes back to our previous discussion about emacs nil of '() vs getting nil via the lack of key.

cider-nrepl could coerce [] to nil as part of it's process. I'm reluctant to change orchard to adopt emacs semantics in place of clojure's.

Having said that, orchard should probably define a behavior for empty include/exclude-meta lists. I'm not sure what reasonable behavior is, my personal view is that :include [] means "include nothing" (nonsensical), and :exclude [] is the same as :exclude nil.

Given that both those states don't mean much or add anything, I consider an empty list "undefined behavior" for now, until I can figure out what it means to be in those states.

[bbatsov]

@vspinu Any great ideas how to properly encode nil on the Emacs side as nil? Maybe add some special representation for it for our bencode encoder?

[SevereOverfl0w]

https://github.com/clojure-emacs/cider-nrepl/blob/2e7be20d3ed9afb0ef952529bbb1b7eace6712da/src/cider/nrepl/middleware/util/coerce.clj#L40-L41 adding a seq here, like done for https://github.com/clojure-emacs/cider-nrepl/blob/2e7be20d3ed9afb0ef952529bbb1b7eace6712da/src/cider/nrepl/middleware/util/coerce.clj#L37 should do the trick.

[vspinu]

I am not following all the details, but would not sending nil entries be an option? On the other side you would essentially get a nil.

[vspinu]

I'm reluctant to change orchard to adopt emacs semantics in place of clojure's.

It's actually nrepl/bencode semantics which identifies lists and nils. So if you would like to support nrepl clients I am afraid you have to either adopt this semantics, push a change to tools.nrepl.bencode, or enforce nil-punning which orchard's query.clj already does with that cond->>.

[tychedelia]

Same problem running via the Spacemac's Clojure layer. Can't run any of my tests. Not doing anything with include / exclude.

;; CIDER 0.18.0snapshot (package: 20180629.1511), nREPL 0.2.13
;; Clojure 1.9.0, Java 1.8.0_172

[bbatsov]

@vspinu is right. The best fix is not to send the nil values to the middleware. I'll update this.

It's actually nrepl/bencode semantics which identifies lists and nils. So if you would like to support nrepl clients I am afraid you have to either adopt this semantics, push a change to tools.nrepl.bencode, or enforce nil-punning which orchard's query.clj already does with that cond->>.

I never read nREPL bencode implementation, I just assumed the problem we were facing with the nils was related to the fact they don't truly exist in Emacs.

[bbatsov]

@jmcelwain @jumarko Can you confirm that this is solved for you?

[jumarko]

@bbatsov I don't see any change. When I use :integration as "excludes" cider still runs all the tests.
I have following cider version:

;; CIDER 0.18.0snapshot (package: 20180714.811), nREPL 0.2.13
;; Clojure 1.9.0, Java 10

When I use :integration as "includes" then cider runs no tests at all.

[bbatsov]

Please, file a different issue for this, so we won't forget about this.

[jumarko]

Ok, so after reading #2357 (comment) properly I can confirm that this works.
At least for the case when you specify "include" explicitly (e.g. dummy).

(deftest ^:dummy dummy-test
  (is (= 1 0)))

(deftest ^:happy happy-test
  (is (= 1 1)))

One more problem that I had was using :dummy selector instead of dummy.
This could be mentioned in the documentation: http://docs.cider.mx/en/latest/running_tests/

[bbatsov]

Probably we should just support both, so people won't have to think about this.

[bbatsov]

Done.