New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ClojureScript support (using cljc) #15

Merged
merged 10 commits into from Sep 15, 2015

Conversation

Projects
None yet
2 participants
@nberger
Collaborator

nberger commented Aug 4, 2015

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

Status

  • test.chuck.properties
  • test.chuck.generators (except double & regexes)
  • test.chuck.clojure-test
  • Implement a better test runner to avoid calling (run-tests) in each cljs test ns, and also to run tests in the browser.
  • 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

This comment has been minimized.

Collaborator

nberger commented Aug 4, 2015

@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

This comment has been minimized.

Collaborator

nberger commented Aug 4, 2015

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

This comment has been minimized.

Owner

gfredericks commented Aug 4, 2015

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

This comment has been minimized.

Collaborator

nberger commented Aug 4, 2015

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

This comment has been minimized.

Collaborator

nberger commented Aug 4, 2015

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

@gfredericks

This comment has been minimized.

Owner

gfredericks commented Aug 4, 2015

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

This comment has been minimized.

Collaborator

nberger commented Aug 4, 2015

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

This comment has been minimized.

Owner

gfredericks commented Aug 4, 2015

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

@nberger

This comment has been minimized.

Collaborator

nberger commented Aug 4, 2015

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

This comment has been minimized.

Collaborator

nberger commented Aug 4, 2015

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

This comment has been minimized.

Collaborator

nberger commented Aug 5, 2015

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 comment has been minimized.

Owner

gfredericks commented Aug 5, 2015

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

This comment has been minimized.

Collaborator

nberger commented Aug 5, 2015

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 😃).

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

@gfredericks

This comment has been minimized.

Owner

gfredericks commented Aug 5, 2015

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

This comment has been minimized.

Collaborator

nberger commented Aug 5, 2015

Sure, I'll do.

@nberger

This comment has been minimized.

Collaborator

nberger commented Aug 5, 2015

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

This comment has been minimized.

Collaborator

nberger commented Aug 5, 2015

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

This comment has been minimized.

Owner

gfredericks commented Aug 6, 2015

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

@nberger

This comment has been minimized.

Collaborator

nberger commented Aug 6, 2015

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

project.clj Outdated
"com.gfredericks.test.chuck.clojure-test-test"
"com.gfredericks.test.chuck.generators-test"
"com.gfredericks.test.chuck.properties-test"
"com.gfredericks.test.chuck.regexes-test"]

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

this test alias shouldn't be needed anymore, lein 2.5.2 supports cljc files.

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

removed in 21dd3dd

project.clj Outdated
@@ -3,12 +3,37 @@
:url "https://github.com/fredericksgary/test.chuck"
:license {:name "Eclipse Public License"
:url "http://www.eclipse.org/legal/epl-v10.html"}
:dependencies [[org.clojure/clojure "1.6.0"]
:dependencies [[org.clojure/clojure "1.7.0"]
[org.clojure/clojurescript "0.0-3308"]

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

cljs has a new 1.7.something version series now

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

although now that I think about it, cljs should probably be a dev dependency. Else anybody using test.chuck will now have cljs+etc on the classpath, which is complicated and sometimes bad.

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

what about :scope "provided"? core.logic does it that way. It means cljs jar will not be included in test.chuck uberjar. I'm not sure if it also means it won't be installed as a transitive dependency, but I think so. I'll check that now, and also bump to cljs 1.7.xx

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

Bumped to clojurescript 1.7.48

Also added :scope "provided" to both clj & cljs. I checked and it works fine in that it doesn't install clojurescript as a transitive dependency when declaring the test.chuck dependency. And being strict, cljs is not a dev dependency, it's a "core" dependency, it's just that it's not always used, right?

But anyways, I'm okay if you still prefer to move it to :dev, just let me know.

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

:provided sounds fine, especially if there's precedent for multi-platform libraries.

I don't care at all about test.check uberjars, I don't think that's a use case for anybody. I was only worried about transitive dependencies, both because it's annoying/noisy and also because there's more possibility for version conflicts e.g. if you're using any of the dependencies that cljs itself uses.

@@ -0,0 +1,54 @@
(ns com.gfredericks.test.chuck.cljs-test

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

I'm a bit iffy on the namespace name. test.check will be changing its namespace from clojure.test.check.cljs-test to clojure.test.check.clojure-test soon I think, so it might make sense for us to do the same thing.

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

We have separate namespaces exactly because of the separation in test.check: as they have separate ns, we need to refer to cljs-test.check.properties/for-all and clojure-test.check.properties/for-all in our macro depending if we are compiling for clj or cljs, and we couldn't find the way to do that from a single macro.

Another solution is to have the two macros in the same namespace with different names. That's worst than the current scenario IMO, because it forces the user to use reader conditionals every time he was to use the macro, instead of only in the ns declaration.

So I guess we have two options:

  1. Wait for the release of test.check where cljs-test ns is unified with clojure-test.
  2. Keep the separate ns. When test.check changes its namespaces, test.chuck will follow (perhaps keeping the cljs-test ns with a deprecation notice until the next major release...)
(defn ^:private not-falsey-or-exception?
[value]
(and value (not (instance? #?(:clj Throwable
:cljs js/Error)

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

oh this is interesting -- js let's you throw arbitrary objects, doesn't it? Does that mean that we can't currently distinguish a thrown not-js/Error-object from a returned object?

There are some changes in mind to test.check that would make this a bit cleaner, and I'm okay living with this for now, I just wanted to check if that's indeed the case.

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

and test.check even on clj already has a converse problem where your test can't return (truthy) Exception objects without them being confused for thrown exceptions, so this is definitely okay for now.

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

Thanks for pointing this out. I think you are correct. Related to that, we are not being able to report a :failed test due to an exception in an assertion as a distinct result from an :errored test due to an exception not in an assertion. That can be seen in our test. It seems to have its roots in test.check not distinguishing between failure and error, which is not a trivial topic given shrinking is involved.

@@ -78,7 +82,8 @@
(let [pairs (core/partition 2 v1)
names (map first pairs)
gens (map second pairs)]
`(for [[~@names] (gen/tuple ~@gens)
`(for [[~@names]
(~'gen/tuple ~@gens)

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

I'm still a bit confused about how cljs macros fit in everywhere, but my guess is that this is dangerous -- does it not assume that every namespace that calls this macro has done (:require [clojure.test.check.generators :as gen])?

I can see how it would be harder to do generally though, because the generators namespace is different on the different platforms, is that right? If that's the biggest blocker, I've plans to rename the cljs namespaces in test.check, so I could expidite that.

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

Yes, you are correct. This is wrong, has to be fixed in a different way. We can't use the ns alias with quote-unquote, at least it should be the full ns name.

My worry is that we are going to play the "separate macros for clj and cljs" dance here too.

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

but is it true that if test.check had unified namespace names the problem would go away?

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

yes :)

@@ -144,7 +149,8 @@
collection, in the same order. For collections of distinct elements
this is effectively a subset generator, with an ordering guarantee."
[elements]
(for [bools (apply gen/tuple (repeat (count elements) gen/boolean))]
(for [bools (apply gen/tuple
(repeat (count elements) gen/boolean))]

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

I won't be a stickler for this, but I'd prefer to avoid whitespace changes; don't mind a separate PR for just whitespace though.

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

Whitespace change removed. This is especially important given this is already a quite big PR.

(gen'/for ~bindings
(with-meta
~(zipmap quoted-names bound-names)
{::for-all-bindings-map true}))]

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

this metadata is probably unnecessary; I can't see any other reference to that keyword. But that could be a separate commit.

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

Yes, it's not being used anywhere in the project. When it was introduced it was supposed to be used by tooling. We could try asking the guy who did that commit :P, or if he doesn't remember if he ended using it, we could just remove it and see if someone complains. I couldn't find a reference to it anywhere in github

But yes, I think it's better to remove it in a separate commit, because we would remove it from clj (already released) and cljs side, right?

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

it might have been my idea; I had forgotten about the tooling angle. Probably just keep it then.

#?(:cljs (set! *print-fn* js-print))
#?(:cljs (run-tests))

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

doesn't test.check have a global test-runner file that handles stuff like this and runs the other files?

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

Yes. I started with those explicit (run-tests) because it was very easy to start rolling, but it's definitely something to improve. I'll take the test.check approach.

This comment has been minimized.

@nberger

nberger Aug 10, 2015

Collaborator

Instead of the test.check approach, what do you think about using https://github.com/bensu/doo? I gave it a try and it's working in my "doo" branch: https://github.com/nberger/test.chuck/tree/doo

The advantage I see is basically that there's less boilerplate, and we get more runners (slimer.js and rhino) for "free".

[cljs.test.check :as t.c]
[cljs.test.check.generators :as gen]
[cljs.test.check.properties :include-macros true]
[cljs.test.check.cljs-test :refer-macros [defspec]])))

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

this makes me want to just go release the renamed test.check namespaces right now.

This comment has been minimized.

@nberger

nberger Aug 9, 2015

Collaborator

Yes, please do it :). Btw is there a test.check branch or patch somewhere doing that?

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

working on it; I'll most likely make another RC release shortly

This comment has been minimized.

@gfredericks

gfredericks Aug 9, 2015

Owner

Okay there's a branch up called cljs-namespace-renaming that you ought to be able to play with. I'm going to try it on one of my own projects and release an RC if it works

This comment has been minimized.

@nberger

nberger Aug 10, 2015

Collaborator

Cool, I'll give it a try. I see it's already in master, so I guess it worked on your project.

This comment has been minimized.

@gfredericks

gfredericks Aug 10, 2015

Owner

0.8.0-RC3 is in the oven and should be available within 20 minutes or so. I fixed the problem with latest cljs, so you shoud be able to use 1.7.48 as well.

This comment has been minimized.

@nberger

nberger Aug 10, 2015

Collaborator

Thanks. I'm making progress in using the unified namespaces: generators-test is working, properties-test is closer to be working. I should be able to push something or report issues soon.

@gfredericks

This comment has been minimized.

Owner

gfredericks commented Aug 10, 2015

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

This comment has been minimized.

Collaborator

nberger commented Aug 10, 2015

@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

This comment has been minimized.

Collaborator

nberger commented Sep 8, 2015

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

@nberger

This comment has been minimized.

Collaborator

nberger commented Sep 9, 2015

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

This comment has been minimized.

Collaborator

nberger commented Sep 9, 2015

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.

`(prop/for-all [{:syms [~@bound-names]}
(gen'/for ~bindings
`(clojure.test.check.properties/for-all [{:syms [~@bound-names]}
(com.gfredericks.test.chuck.generators/for ~bindings

This comment has been minimized.

@gfredericks

gfredericks Sep 10, 2015

Owner

this didn't need to change either?

This comment has been minimized.

@nberger

nberger Sep 10, 2015

Collaborator

Yes, I changed this and a couple more places where an alias could be used in 5ad5d11

:cljs
(binding [*current-env* (test/empty-env)]
(capture-test-var #'this-test-should-crash-and-be-caught)
(:report-counters *current-env*)))]

This comment has been minimized.

@gfredericks

gfredericks Sep 10, 2015

Owner

the logic represented by this #?(...) in duplicated in the next test, and is hard to read in this context. Could it be extracted into a function whose name describes what it's doing? (which I assume is running the test and returning the results)

This comment has been minimized.

@nberger

nberger Sep 10, 2015

Collaborator

Done in a01278f

(tc/quick-check 100
(prop'/for-all [x gen/int]
#?(:clj (/ 4 0)
:cljs (js/Error. "Oops"))))))))

This comment has been minimized.

@gfredericks

gfredericks Sep 10, 2015

Owner

you're not throwing the error here

This comment has been minimized.

@gfredericks

gfredericks Sep 10, 2015

Owner

there's gotta be a platform-agnostic way to make something crash :/ what does (subs nil 1 2) do in cljs?

This comment has been minimized.

@nberger

nberger Sep 10, 2015

Collaborator

Yes, there must be some way, for sure. I'll take a look tomorrow

This comment has been minimized.

@nberger

nberger Sep 10, 2015

Collaborator

Changed it to (in a9c6904):

(checking "subs on any string or nil" 100
  [s (gen/one-of [gen/string (gen/return nil)])]
  ;; going for uncaught-error-not-in-assertion here
  (let [res (subs s 1 2)]
    (is res)))

But also did something similar in exception-handling-test and now I get {:pass 1, :fail 1, :error 0} instead of {:pass 0, :fail 1, :error 0} in the exception-detection-test in cljs. In clj works fine as before, returning {:pass 0, :fail 1, :error 0}.

Not sure why this is happening, but I don't want to fix it just by changing it to a different expression or something like that... I want to understand it first.

Any ideas?

This comment has been minimized.

@nberger

nberger Sep 10, 2015

Collaborator

Cljs tests are running in circle now, failing of course because of this issue :/

https://circleci.com/gh/nberger/test.chuck/26

(binding [*current-env* (test/empty-env)]
(capture-test-var #'this-test-should-fail)
[(:report-counters *current-env*) nil]))]
#?(:clj (is (not (re-find #"not-falsey-or-exception" out))))

This comment has been minimized.

@gfredericks

gfredericks Sep 10, 2015

Owner

does with-out-str not work in cljs?

This comment has been minimized.

@nberger

nberger Sep 10, 2015

Collaborator

Yes, it works. It was fixed as part of a01278f

@nberger nberger force-pushed the nberger:cljc branch 3 times, most recently from 18ad083 to 2cca4af Sep 10, 2015

[com.gfredericks.test.chuck.generators :as gen']))
[clojure.test.check.properties :as prop
#?@(:cljs [:include-macros true])]
[com.gfredericks.test.chuck.generators :as gen

This comment has been minimized.

@gfredericks

gfredericks Sep 11, 2015

Owner

I'd like to keep the alias here as gen'; I use gen for clojure.test.check.generators.

This comment has been minimized.

@nberger

nberger Sep 11, 2015

Collaborator

Done

(defn -report
[reports]
(#?(:clj clojure.test/report :cljs cljs.test/report) reports))

This comment has been minimized.

@gfredericks

gfredericks Sep 12, 2015

Owner

If we used a namespace alias for clojure.test and cljs.test (i.e., the same alias), we wouldn't need the reader conditional here would we?

This comment has been minimized.

@nberger

nberger Sep 12, 2015

Collaborator

Yes. It's done in 39c3faf

;;
;; I think there might be plans for test.check to abstract this logic
;; into a protocol or something, so I'm not too bothered by the
;; copypasta for now.

This comment has been minimized.

@gfredericks

gfredericks Sep 12, 2015

Owner

did this comment get removed on master? I think it only applied to not-falsey-or-exception? which doesn't seem to be what we're using anymore

This comment has been minimized.

@gfredericks

gfredericks Sep 12, 2015

Owner

doesn't look like you addressed this one

This comment has been minimized.

@nberger

nberger Sep 12, 2015

Collaborator

Ups. Taking a look now...

This comment has been minimized.

@nberger

nberger Sep 12, 2015

Collaborator

You are right, it was removed on master. It slipped through here... I just removed it in 8f978c6

@nberger nberger force-pushed the nberger:cljc branch from 94bec29 to 39c3faf Sep 12, 2015

@nberger

This comment has been minimized.

Collaborator

nberger commented Sep 12, 2015

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.

@nberger nberger force-pushed the nberger:cljc branch from 39c3faf to 7471a0c Sep 12, 2015

@nberger

This comment has been minimized.

Collaborator

nberger commented Sep 12, 2015

Just pushed the rebased commits.

I kept 7 commits, not just 1, as I think it might be of value to keep those separate in the history. But let me know if you think we should squash some or all of them.

(defn -testing
[name func]
(#?(:clj clojure.test/testing :cljs cljs.test/testing) name (func)))

This comment has been minimized.

@gfredericks

gfredericks Sep 13, 2015

Owner

whoops I meant to have this changed too, to ct/testing

This comment has been minimized.

@nberger

nberger Sep 13, 2015

Collaborator

whoops I meant to have this changed too, to ct/testing

I guessed so, and tried to change it too, but it didn't work. It was
because testing is a macro, and this triggers the original problem we had
with trying to refer to macros in different namespaces in clj and cljs...

So, I'm not saying this is impossible to do, but it's not trivial, and it
can be done after merging this PR :D

This comment has been minimized.

@gfredericks

gfredericks Sep 13, 2015

Owner

is it not just because testing isn't mentioned in :refer-macros at the top? (or that you didn't use :include-macros true?)

This comment has been minimized.

@nberger

nberger Sep 13, 2015

Collaborator

is it not just because testing isn't mentioned in :refer-macros at the
top? (or that you didn't use :include-macros true?

No. AFAIK when there exists a clj ns with the same name as the cljs ns, all
of its macros are available without the need of :refer-macros or anything.
It's explained in the "differences from clojure" page in the cljs wiki, but
I'm with bad connection now and can't find it

This comment has been minimized.

@nberger

nberger via email Sep 13, 2015

Collaborator

This comment has been minimized.

@nberger

nberger Sep 13, 2015

Collaborator

Sorry, we are talking about another ns here... In that case, I tried
adding :refer-macros and it didn't work :)

Or perhaps I didn't :(.

I'll try again tomorrow

This comment has been minimized.

@nberger

nberger Sep 14, 2015

Collaborator

Well, of course you were right, and I was wrong. It worked with :refer-macros [testing]. It's done in 23f34e3.

I just checked and what I tried the other day was [cljs.test :as ct :refer-macros [is testing]] but then tried to call the macro as (ct/testing ...) instead of (testing ...).

@nberger

This comment has been minimized.

Collaborator

nberger commented Sep 14, 2015

@gfredericks I just updated test.check dependency to 0.8.2

Let me know if you find anything else :)

@gfredericks gfredericks merged commit 31ea8ca into gfredericks:master Sep 15, 2015

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment