clojure-emacs / cider-nrepl

A collection of nREPL middleware to enhance Clojure editors with common functionality like definition lookup, code completion, etc.
https://docs.cider.mx/cider-nrepl
684 stars 176 forks source link

`cider-test-run-ns-tests` not respecting `test-ns-hook` #680

Open jarohen opened 4 years ago

jarohen commented 4 years ago

Hey @bbatsov et al, hope you're all keeping well :)

Let me know if more information would be helpful, think this is my first CIDER bug report :)

Cheers,

James

Use the template below when reporting bugs. Please, make sure that you're running the latest stable release or the latest snapshot/pre-release of cider-nrepl and that the problem you're reporting hasn't been reported (and potentially fixed) already.

Expected behavior

If the namespace contains a test-ns-hook function, cider-test-run-ns-tests should call this instead of directly calling whatever test functions the namespace may contain

Actual behavior

It doesn't - in my case (because the namespace has no concrete tests) it fails with 'No assertions (or tests) were run. Did you forget to use is in your tests?'

Steps to reproduce the problem

(ns foo.bar_test
  (:require [clojure.test :as t])

(defn test-ns-hook []
  (t/is (= 1 1)))

Calling (clojure.test/test-ns 'foo.bar-test) directly does check the (= 1 1) assertion.

Environment & Version information

Spacemacs develop branch

;; CIDER 1.0.0snapshot (package: 20200916.1152), nREPL 0.7.0
;; Clojure 1.10.1, Java 1.8.0_265

[cider/cider-nrepl "0.25.3"]

Operating system

Arch Linux

jarohen commented 4 years ago

Looking into this in more detail, this message seems to be going via handle-test-var-query-op and then test-var-query - (query/vars var-query) is returning an empty seq (correctly, because there's no concrete test vars in that namespace). IIUC, this code-path is missing a test-ns-hook check? Not sure where it should live, though!

jarohen commented 4 years ago

Oh - cider.nrepl.middleware.test/test-ns (directly above) would check for test-ns-hook, but it never gets called, because the doseq in test-var-query is running over an empty seq.

HTH!

bbatsov commented 4 years ago

Thanks for the detailed report! If you'd like to take a stab at solving this yourself that'd be great. Seems the solution would be pretty-straightforward.

jarohen commented 4 years ago

Hey @bbatsov, thanks for getting back to me :)

Have had a bit of a bash at this today, struggling a little with the semantics. I was aiming for the same semantics as clojure.test - i.e. when you call c.t/test-ns (cider-test-run-ns-tests) it checks for the hook; when you call c.t/test-var (cider-test-run-test) it doesn't - but this doesn't seem to be how CIDER behaves currently? If so, would you prefer to preserve CIDER's current semantics, or would you be happy to make this consistent?

I might be focusing in the wrong area, but I also seem to be running into Orchard's query namespace, working against the has-tests? and test? flags on the vars function - would a change to Orchard be palatable if need be?

Cheers,

James

bbatsov commented 3 years ago

If so, would you prefer to preserve CIDER's current semantics, or would you be happy to make this consistent?

I'd be happy to make this consistent.

I might be focusing in the wrong area, but I also seem to be running into Orchard's query namespace, working against the has-tests? and test? flags on the vars function - would a change to Orchard be palatable if need be?

Yeah, absolutely.

jarohen commented 3 years ago

Hey @bbatsov, cheers! I'm afraid it might be a while until I get back around to this one - we've recently had a baby so don't have a lot of spare brain cycles at the moment :smile: Thankfully the workaround in the commit above is fairly straightforward for anybody else stumbling on this issue, and the fix doesn't seem too involved - maybe someone else reading this might be able to pick up the baton :)

James

bbatsov commented 3 years ago

I'm afraid it might be a while until I get back around to this one

No rush!

we've recently had a baby so don't have a lot of spare brain cycles at the moment

Congratulations! 🎉 🎈 🍾

jarohen commented 3 years ago

thanks! :pray:

hodgiwabi commented 3 years ago

Since I'm working with cider-test-run-ns-tests in cider#2958 I can also try to look into this. It looks a bit more involved, and may take me some extra time because I'm new, but I would be happy to take up the torch.