Add ClojureScript support (using cljc)

[nberger]

Alternative version of #14, with bump to clojure 1.7.0 to use reader conditionals.

Status

• [x] test.chuck.properties
• [x] test.chuck.generators (except double & regexes)
• [x] test.chuck.clojure-test
• [x] Implement a better test runner to avoid calling (run-tests) in each cljs test ns, and also to run tests in the browser.
• [x] Improve lein test-all (and/or add a new "test" alias) to compensate the issue of leiningen not picking up .cljc files. The namespaces have to be explicitly enumerated. See technomancy/leiningen#1940

[nberger]

@gfredericks: this is WIP yet, but I wanted to open the PR so you (or anyone) can take a look early.

I tried different strategies to implement the properties/for-all macro for cljs, and the best alternative was to move the cljs version to a separate ns, so the users only need to use a reader conditional in the ns declaration. There's an intermediate commit using a separate macro name for cljs, but this way is clearly better IMO.

I'm working now on the test.chuck.clojure-test and its tests, and looks like it's a similar scenario with the three macros there.

[nberger]

Also, I opened a separate PR with the idea to close the original, but perhaps I should just push it to #14. Please let me know which one do you think is better :)

[gfredericks]

Re: properties/for-all: Shouldn't we be able to prevent users from having to use their own reader conditionals by using one ourselves in the actual impl?

[nberger]

Re: properties/for-all: Shouldn't we be able to prevent users from having to use their own reader conditionals by using one ourselves in the actual impl?

I thought we should, but couldn't find how. Found an explanation in the macros section of https://www.paren.com/posts/isomorphic-clojure-part-2-portable-code that makes much sense:

Since ClojureScript macros are really Clojure macros, symbols are resolved in Clojure. For example, map would resolve to clojure.core/map instead of cljs.core/map. The only way around this faulty resolution is to quote and then unquote the symbol, like ~'map.

[nberger]

And I understand Leon Grapenthin is referring to exactly that in his response (about this post) in this thread

[gfredericks]

I don't mean that you'd necessarily have one (defmacro for-all ...); you could, if necessary, have two of them in the same file, with a reader conditional at the top level. So same amount of code, just organized in a user-friendlier way. I might be wrong about how the reader conditional works for cljs macros though :/

I feel like there's gotta be a way to make it work one way or another, I just don't have time to think it through right this second.

[nberger]

I don't mean that you'd necessarily have one (defmacro for-all ...); you could, if necessary, have two of them in the same file, with a reader conditional at the top level. So same amount of code, just organized in a user-friendlier way. I might be wrong about how the reader conditional works for cljs macros though :/

Sadly, reader conditionals won't help in trying to have two (defmacro for-all...) in the same ns, one for cljs and one for clj: that's because defmacro is only valid inside of #?(:clj ...).

[gfredericks]

is there a runtime mechanism for a macro to detect if it's compiling clj code or cljs code?

[nberger]

is there a runtime mechanism for a macro to detect if it's compiling clj code or cljs code?

There is: if (:ns &env). See here. But from what I read, I understand it has some cons.

[nberger]

I discarded that path when I read (from that thread):

When you call a macro from a cljs file, and that macro calls another macro with a(:ns &env) test, it sees a Clojure &env, so it thinks is being compiled in a CLJ environment

I think we are in that situation: both checking and for-all need to know if they are being compiled in cljs, and both call capture-reports which also need to know that. As I am saying this, I think one simple solution to that issue is to inline the capture-reports macro. But I'm still not sure if that's it. We can try it :).

Anyways, I'd first finish with the current strategy, we are very close to have something usable with one gotcha: when used from cljc users will need a reader conditional to use the macros in cljs (which can be refactored (when used directly from a .cljs file, it's "just" that it's a different ns).

[nberger]

test.chuck.clojure-test and test.chuck.cljs-test namespaces are working with tests (except exception-detection-test in cljs until we implement the cljs alternative for (binding [clojure.test/*test-out* (java.io.StringWriter.)] ...)

As of today, we have separate namespaces for cljs & clj macros, so for example in the case of checking and for-all the user will need a ns declaration like:

(ns my.ns-test
  #?(:clj  (:require [clojure.test :refer :all]
                     [clojure.test.check.generators :as gen]
                     [com.gfredericks.test.chuck.clojure-test :refer [checking for-all]])
     :cljs (:require [cljs.test :refer-macros [deftest is testing run-tests]]
                     [cljs.test.check.generators :as gen]
                     [com.gfredericks.test.chuck.cljs-test :refer-macros [checking for-all]])))

(deftest integer-facts
  (checking "positive" 100 [i gen/s-pos-int]
    (is (> i 0)))

  (checking "negative" 100 [i gen/s-neg-int]
    (is (< i 0))))

I'll try to explore the possibility to overcome this by using the runtime mechanism discussed above to detect if the compilation is being done for clj or cljs.

[gfredericks]

This is sounding cool -- is it something that you're personally going to be using?

I haven't actually talked to anybody using test.check in cljs yet.

[nberger]

Nice. I used test.check in a cljs project around 3 months ago, and by a quick look I see it could have made really good use of at least the for-all macro (had to do and on multiple truthy forms instead of multiple is) and gen'/double (used gen/int, so a few test cases were left out :smiley:).

I'm not going to use it right now, but will probably do in the next cljs project.

[gfredericks]

okay cool; on a not-related-to-test.chuck note, I'd be interested if you would try out test.check version 0.8.0-RC2 to make sure it works and especially that it doesn't noticeably slow down your test suite.

[nberger]

Sure, I'll do.

[nberger]

I notice a slow down of around 10-20% in the cljs tests in that project, using 0.8.0-RC1 (couldn't find RC2) vs 0.7.0. I can't say if it's a representative sample: it has only 1 test.check test. I measured with sizes 100 (1.3 vs 1.55 sec), 200 (5.2 vs 6.1 sec) and 1000 (25 vs 29 sec). The slow down seems consistent through the sample sizes.

[nberger]

Of course, that's an ad-hoc benchmark. I can share more details with you, and/or try on different platforms, but better to do it somewhere else, to not lose the focus of this PR :)

[gfredericks]

Thanks for the timing info. I'll be looking at the PR in more detail soon.

[nberger]

Added the cljc version of exception-handling-test.

Merged in #16 to use it as the base implementation, those commits can be removed from here after #16 is merged into master.

@gfredericks: Do you have any idea in mind or preference about the cljsbuild profiles and test runner? Should we have a setup like that in test.check, with browser & node dev/adv profiles? Should we add it as part of this PR?

We currently have a very simple solution taken from core.match which expects every test ns to have the (run-tests) in the end and by using :notify-command the tests run via lein cljsbuild auto adv

[gfredericks]

okay test-check 0.8.0-RC2 is out. You might have to bump cljs down to 1.7.28 to get it to work though. Something about 1.7.48 is bad with the new test.check and I haven't figured out what yet.

[nberger]

@gfredericks: tests are working with 0.8.0-RC3, using the unified namespaces.

But: I still had to keep the two separate namespaces test.chuck.cljs-test and test.chuck.clojure-test. That's because of at least two things:

1. the capture-reports macros which still have some differences between clj & cljs
2. the call to cljs.test/report & clojure.test/report at the end of the checking macro.

I'll if I can find a way to overcome this. But for now, it forces us to have separate checking and for-all macros, thus separate namespaces.

[nberger]

Also: I merged the branch for switching to bensu/doo for the test runner. It includes a small addition to the README explaining how to run the tests. Let me know whether you like it or if you still prefer to take the runners from test.check

[gfredericks]

what is it about the test.chuck.{clojure,cljs}-test namespaces that prevents handling the differences with reader conditionals?

[gfredericks]

I'm not opposed to bensu/doo if it's a solid library and does reduce boilerplate

[nberger]

what is it about the test.chuck.{clojure,cljs}-test namespaces that prevents handling the differences with reader conditionals?

It's the need to refer to different symbols depending on whether it's compiling for cljs or clj. This cannot be done using reader conditionals inside of a macro, because it will always go through the :clj branch.

There's one thing I haven't tried: The (:ns &env) runtime trick (see https://github.com/gfredericks/test.chuck/pull/15#issuecomment-127768642). I'll give it a whirl and let you know.

About lein-doo: I think it's solid, and being just dev tooling we can change it later if we want to, so let's use it :)

[nberger]

@gfredericks: Made the namespace unification by using (:ns &env). It's working. I had to inline the capture-reports macro, and there's a bit of repeated code inside of the macros, but hey, it works! :)

I mean, we are free to refactor the macro code later if we want to, but at least we got rid of the separate namespaces. Thanks for insisting on that.

Let me know how it looks to you.

[nberger]

@gfredericks: moved test.check to the global dependencies vector. We want it pulled as a transitive dependency, so a project would just declare test.chuck to get both, right?

Btw, this change could also be done in master. Do you want me to create a separate PR to do that on the current master?

Also, do you want me to try to extract smaller PR's from this bigger one? Probably won't be able to extract many, but we already have #16 for example.

[gfredericks]

It's the need to refer to different symbols depending on whether it's compiling for cljs or clj. This cannot be done using reader conditionals inside of a macro, because it will always go through the :clj branch.

Dangit I always forget about that

[gfredericks]

I don't think projects would realistically not declare their own test.check dependency, but I suppose they could. It's probably good to have test.check as a real dep anyhow so that we have the version set and users can see conflicts.

[gfredericks]

so with the (:ns &env) approach there was a downside with writing macros that expand to the macro that it appears in, is that right? Is there a workaround where the macro-author could somehow set :ns explicitly?

[nberger]

so with the (:ns &env) approach there was a downside with writing macros that expand to the macro that it appears in, is that right? Is there a workaround where the macro-author could somehow set :ns explicitly?

Yes, seems like that's the downside. Don't know, will investigate more about the :ns thing.

[nberger]

See this excerpt from https://groups.google.com/d/msg/clojurescript/iBY5HaQda4A/SuVYqNRH8gwJ:

macros that want to emit cross-platform code should emit forms like `(if-cljs cljs-form clj-form) -- notice the backtick -- and even if they are called from other macros, the ultimate expansion takes place in the correct target context (I hope).

We are emiting forms like that (with backtick), so we should be fine to call this macros from other macros without the need to set :ns.

I don't see someone building macro sugar on top of the test.chuck macro sugar, but it can be done (test.chick project coming...). But what's important here is that test.check could use a similar strategy for the underlying macros, and test.chuck should be able to call them. Should we start the "refactor to cljc" branch in test.check? :)

[gfredericks]

I do have plans to move test.check to cljc but I thought I might wait till at least the next release to delay requiring a 1.7 upgrade from users.

[nberger]

Sounds reasonable. Would that be the 0.8.1 release?

[gfredericks]

Could we sidestep the cross-platfrom macro problem by factoring most of the functionality out of the macro and into functions?

[nberger]

Could we sidestep the cross-platfrom macro problem by factoring most of the functionality out of the macro and into functions?

I'm not sure if we can refactor into functions in this case, because we do the wrap of the body into a call to a (platform-specific) binding... I tried to extract a function or a macro just to generate the binding vector, but failed. If you can do that, please enlighten me :)

Anyways, your call to refactor made me remember that I inlined the capture-reports macro in the middle of incorporating the if-cljs macro... So I stepped back and extracted that macro again, and looks much better now. It's in the two commits I just pushed.

Next (easy) step is to extract a macro for the report-when-failing call which is the same for both platforms now.

Let me know if you see something I am missing or a better way to do this.

[nberger]

Extracted the qc-and-report-when-failing macro. The platform-specific stuff is now confined to capture-reports and (a much simpler) checking macro.

[nberger]

@gfredericks:

I just merged in master, and implemented gen/datetime for cljs using cljs-time.

As we are still doing reviews I'm keeping the raw commits. We can rebase to master (and squash commits) anytime. I'd do it once we are ready to merge, but that's your call.

[gfredericks]

so is this all done to the best of your knowledge? I'm curious if the latest test.check release has been sufficient for this task; if so I'll probably consider that justification for a full release.

[nberger]

I think so. With regard to namespaces and macros, the latest test.check release has been a great help.

[gfredericks]

planning on looking at this in detail in the next few days. thanks again for all the work.

[nberger]

Ok, thanks.

I just tried to sync with master but I found that a test started to fail. I'll wait for the resolution of #23 before doing the sync.

[gfredericks]

If I don't get to this sooner I will definitely definitely get to it on Tuesday.

[nberger]

Ok, thanks.

I merged this with master.

I also pushed a cljc-rebased branch to my repo with all of this rebased to master, with the commits squashed to just a few. I'm keeping the two branches so as to not interfere with the review, but I think we should merge something like cljc-rebased instead of this :)

[gfredericks]

I hate to ask the same question every three weeks, but I couldn't figure this out from the comment history:

Why do we have a separate .clojure-test.impl namespace?

[gfredericks]

Just did a full scan of the changes, looking pretty good aside from the comments I made, so I think we're close. Will also need another merge from master.

[nberger]

Thanks for the thorough review. I just merged with master, and will take a look into the comments ASAP.

[nberger]

Why do we have a separate .clojure-test.impl namespace?

Good catch! The separation was needed when we had separate macros and namespaces for clj & cljs, so as to reuse that code from both ns, but that's not the case anymore. I just moved the code to .clojure-test and removed .clojure-test.impl

[nberger]

I think I've answered everything :)

I've also rebased my cljs-rebased branch again, which is based on master and includes everything from here. It's only 4 commits versus 48 here, so we should probably make this branch to point to cljc-rebased before merging :)

To be sure both branches have the same content, I'm running git diff cljc-rebased cljc. github compare view doesn't work in this case.

[nberger]

Everything seems to be ok now. I'm going to update the rebased branch with the latest changes and push it here in a few minutes.