Make checking's options optional
#58
Conversation
|
Sorry not finished, I should update all documentation. |
|
Documentation updated, but the I verified, the error doesn't happen in the |
|
Can't find the problem with CLJS tests. And my knowledge of leiningen is very limited so I didn't find an easy way to launch a CLJS REPL to further investigate the problem. Hopefully I'm close to the solution. Now I gotta go back to work. If you're willing to have a look you're welcome. |
| [name & check-decl] | ||
| (let [[num-tests-or-options bindings body] (cond | ||
| (and (or number? (first check-decl) | ||
| map? (first check-decl)) |
There was a problem hiding this comment.
you're missing parens around these calls, so this part will always be truthy; that could be your bug
| (-report r#)))))) | ||
| :arglists '([name bindings body] [name num-tests-or-options bindings body])} | ||
| [name & check-decl] | ||
| (let [[num-tests-or-options bindings body] (cond |
There was a problem hiding this comment.
please add a line break before (cond so that this clause isn't so far off to the right
| @@ -106,27 +105,44 @@ | |||
| (ct/report reports)) | |||
|
|
|||
| (defmacro checking | |||
| "A macro intended to replace the testing macro in clojure.test with a | |||
| ^{:doc "A macro intended to replace the testing macro in clojure.test with a | |||
There was a problem hiding this comment.
I don't think this metadata will be effective unless you delete the ^ character on this line
| ;; empty map is OK, defaults to 100 tests | ||
| (checking "strings are strings" {} [s gen/string-ascii] | ||
| (is (string? s))) | ||
| (is (string? s))) |
There was a problem hiding this comment.
try not to change existing indentation; I'd also prefer the checking call farther up to be indented 2 spaces as well
| [nil (first check-decl) (next check-decl)] | ||
|
|
||
| :else (throw #?(:clj (IllegalArgumentException. "Arguments to `checking` must be either [name bindings body] or [name num-tests-or-options bindings body]") | ||
| :cljs (js/Error. "Arguments to `checking` must be either [name bindings body] or [name num-tests-or-options bindings body]")))) |
There was a problem hiding this comment.
you can avoid the duplication here by pushing the #? down to just the IllegalArgumentException. and js/Error.
| (swap! nb-runs inc) | ||
| (is (> 0))) | ||
| (swap! nb-runs inc) | ||
| (is (> 0))) |
There was a problem hiding this comment.
this isn't really core to the test, but I assume you meant (> i 0) or (pos? i) here?
|
|
your exception is thrown at macroexpansion-time (which makes sense to me), so the fact that it "doesn't even compile" seems like the correct behavior, and I would question why it does anything different in other environments. |
|
|
||
| You can optionally pass in a map options instead of the number of tests, | ||
| You can optionally pass in a number of a map options, |
There was a problem hiding this comment.
"a number or a map of options"
you could perhaps also add "similar to defspec"
|
oh I didn't notice that you were trying to have an automated test that checks the compile-time error. I think the only reasonable way to test this is by calling |
|
I fixed the |
|
Something like |
|
PS: don't know how to display syntax-quote inside markdown code markers since they're back-quotes too ! |
|
You can do syntax-quote in markdown by using the multiline version: https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet#code |
| (prop/for-all ~bindings | ||
| (let [reports# (capture-reports ~@body)] | ||
| (swap! ~final-reports save-to-final-reports reports#) | ||
| (pass? reports#))) |
There was a problem hiding this comment.
please maintain the 2-space indentation for for-all expressions
| (let [reports# (capture-reports ~@body)] | ||
| (swap! ~final-reports save-to-final-reports reports#) | ||
| (pass? reports#))) | ||
| (apply concat (options ~num-tests-or-options))))) |
There was a problem hiding this comment.
The reason there was a let clause at the top in the old code is that it lets you use num-tests-or-options in two places without inserting the code twice. This is general good-practice when writing a macro because otherwise if users have a nontrivial expression, it will be evaluated twice.
E.g., compare the expansions of (doubler (inc 20)) with these two definitions:
(defmacro doubler
[x]
`(+ ~x ~x))
(defmacro doubler
[x]
`(let [x# ~x] (+ x# x#)))
There was a problem hiding this comment.
Now that I look closer, have you actually changed anything in this code, other than inlining num-tests-or-options? If not, then you should be able to just revert to the original code.
There was a problem hiding this comment.
Never thought of the double evaluation. Thanks for the tip.
|
|
||
| :else (throw (#?(:clj IllegalArgumentException. | ||
| :cljs js/Error.) "Arguments to `checking` must be either [name bindings & body] or [name num-tests-or-options bindings & body]"))) | ||
| num-tests-or-options (tc.clojure-test/process-options num-tests-or-options)] |
There was a problem hiding this comment.
the fact that you're calling process-options with the form at macroexpansion-time means that users can't have a non-literal expression; e.g., I think (checking "something" (assoc some-defaults :num-tests 40) ...) would fail (you could add a test for this).
Now that I think about that distinction, I believe your check for number? and map? farther up are not sufficient, since it could also be a symbol or sequence. I think the cleanest thing would just be to only check (vector? (second check-decl)) there.
There was a problem hiding this comment.
Done, but... (read the comment below)
|
Ok, I had some trouble just with the commit "macro problems". My understanding was that since I moved the The "quote binding" commit is an attempt to circumvent the problem but I'm not sure it's a good idea. By the way at this point all tests ( |
|
The standard way of solving the problem you're describing is using I'm shocked that the "quote binding" commit actually works. There must be something very strange going on. |
|
I've just tried several things to be able to use (eval `(checking "numbers are numbers" "opts as string" [i# gen/int]
(is (int? i#))))(eval `(checking "numbers are numbers" "opts as string" [i# gen/int]
(is ('int? i#))))(eval `(checking "numbers are numbers" "opts as string" [i# gen/int]
(is (~'int? i#))))Nothing works. I think I'm lost in macro-hell... haha |
|
The first one should work. How does it fail? |
|
Sorry I should have been more precise, apologies if you took it bad. Below the complete stacktrace from |
|
I think that can happen if It looks like |
|
|
|
Good catch, I changed to |
|
The reason it's not throwing the exception you wanted is that your |
Details in issue #57.