test.check

clojure.test reporting is uninformative

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:

Description

The clojure.test reporting results in output like:

FAIL in (always-fail) (clojure_test.clj:18)
expected: result
  actual: false

Note that the file and line number are always clojure_test.clj:18 and the expected value is always the literal string "result".

I'm not sure what the right output would be for expected/actual, but the incorrect file and line reporting means that clojure-test-mode consistently highlights the wrong line.

Activity

Hide
Philip Potter added a comment -

I think this is because the assertion is checked by calling clojure.test/is. clojure.test/is is being called for its return value, but it has the side effect of reporting failures. It's a macro, and it takes the literal symbol "result" as its expected expression, and takes the current file and line number to attach to the failure. It's really designed to be called by user code, not library code.

Show
Philip Potter added a comment - I think this is because the assertion is checked by calling clojure.test/is. clojure.test/is is being called for its return value, but it has the side effect of reporting failures. It's a macro, and it takes the literal symbol "result" as its expected expression, and takes the current file and line number to attach to the failure. It's really designed to be called by user code, not library code.
Hide
John Walker added a comment -
Show
John Walker added a comment - Well, one solution would involve taking advantage of the report functionality. https://github.com/clojure/clojure/blob/master/src/clj/clojure/test.clj#L204 https://github.com/clojure/clojure/blob/master/src/clj/clojure/test.clj#L371
Hide
Michael Glaesemann added a comment -

This patch relies on the refactoring in TCHECK-132.

This adds a clojure.test/assert-expr which parses the check results
and emits appropriate messages via clojure.test/do-report. For failed
tests, the arguments to clojure-test/assert-check are returned as the
:actual result, with a placeholder {:result true} as the :expected result.

The file and location provided in ClojureScript environments is not
particularly useful as it reports the file and line location in the
relevant JavaScript file rather than of the source ClojureScript file.

The patch includes tests.

Show
Michael Glaesemann added a comment - This patch relies on the refactoring in TCHECK-132. This adds a clojure.test/assert-expr which parses the check results and emits appropriate messages via clojure.test/do-report. For failed tests, the arguments to clojure-test/assert-check are returned as the :actual result, with a placeholder {:result true} as the :expected result. The file and location provided in ClojureScript environments is not particularly useful as it reports the file and line location in the relevant JavaScript file rather than of the source ClojureScript file. The patch includes tests.
Hide
Gary Fredericks added a comment - - edited

Michael Glaesemann thanks for the patch! I think this is a good approach, I just have three comments:

1. Instead of adding three new namespaces over four new files, is it possible to condense it to a single cljc file with one namespace? I'm pretty sure a separate namespace for cljs macros isn't necessary with cljc. I think the name clojure.test.check.clojure-test.assertions would be good.
2. There's a new test can-report-shrinking, and I can't tell if it's related to this change or was included accidentally.
3. The can-report-failures test could also check the printed filename; I think the file & line number is the best part of this change, so it makes sense to me to test it.

Show
Gary Fredericks added a comment - - edited Michael Glaesemann thanks for the patch! I think this is a good approach, I just have three comments: 1. Instead of adding three new namespaces over four new files, is it possible to condense it to a single cljc file with one namespace? I'm pretty sure a separate namespace for cljs macros isn't necessary with cljc. I think the name clojure.test.check.clojure-test.assertions would be good. 2. There's a new test can-report-shrinking, and I can't tell if it's related to this change or was included accidentally. 3. The can-report-failures test could also check the printed filename; I think the file & line number is the best part of this change, so it makes sense to me to test it.
Hide
Michael Glaesemann added a comment -

Thanks for taking a look!

Regarding 1, I also suspect it's possible, but my cross-platform macro-fu isn't good enough to know how to do it. I also am not sure how to ensure it's compatible with bootstrapped CLJS. Does lein cljsbuild test cover that case? I'd like to commit the best code possible, and right now that's the best code I know how to write :) I figure it can be condensed at a later time if someone knows how to do it. Regardless, I'll take another whack at it this weekend.

As for 2, IIRC I added the can-report-shrinking test to characterize the existing behavior which wasn't covered by existing tests. I wanted to ensure I wasn't breaking anything while I was changing the failure reporting. Trying to do Michael Feathers and Uncle Bob proud.

And 3, I agree the file/line reporting is a nice addition, and a check would make sense. I left it out because the reporting in ClojureScript isn't useful. For the existing test, Clojure reports (clojure_test_test.cljc:168) while ClojureScript reports (cljs/test.js:452:85) and (/path/to/my/repo/test.check/target/cljs/node_adv/tests.js:907:262). I'll add an assertion for #"clojure_test_test.cljc:\d+" on the JVM and #"/tests.js:\d+:\d+)" on CLJS.


As an aside, is there a reason the :fail value in the results is wrapped in a vector? It's confused me at times, particularly when I have generators that produce values that are sometimes vectors and sometimes scalars. My preference would be to emit the failed value as-is. Happy to submit a patch--once I'm done with this one, of course :)

https://github.com/clojure/test.check/blob/master/src/main/clojure/clojure/test/check.cljc#L190

Show
Michael Glaesemann added a comment - Thanks for taking a look! Regarding 1, I also suspect it's possible, but my cross-platform macro-fu isn't good enough to know how to do it. I also am not sure how to ensure it's compatible with bootstrapped CLJS. Does lein cljsbuild test cover that case? I'd like to commit the best code possible, and right now that's the best code I know how to write :) I figure it can be condensed at a later time if someone knows how to do it. Regardless, I'll take another whack at it this weekend. As for 2, IIRC I added the can-report-shrinking test to characterize the existing behavior which wasn't covered by existing tests. I wanted to ensure I wasn't breaking anything while I was changing the failure reporting. Trying to do Michael Feathers and Uncle Bob proud. And 3, I agree the file/line reporting is a nice addition, and a check would make sense. I left it out because the reporting in ClojureScript isn't useful. For the existing test, Clojure reports (clojure_test_test.cljc:168) while ClojureScript reports (cljs/test.js:452:85) and (/path/to/my/repo/test.check/target/cljs/node_adv/tests.js:907:262). I'll add an assertion for #"clojure_test_test.cljc:\d+" on the JVM and #"/tests.js:\d+:\d+)" on CLJS.
As an aside, is there a reason the :fail value in the results is wrapped in a vector? It's confused me at times, particularly when I have generators that produce values that are sometimes vectors and sometimes scalars. My preference would be to emit the failed value as-is. Happy to submit a patch--once I'm done with this one, of course :) https://github.com/clojure/test.check/blob/master/src/main/clojure/clojure/test/check.cljc#L190
Hide
Gary Fredericks added a comment -

1) there are a few macros in test.check itself (defspec, for-all, gen/let) that might serve as examples
2) that makes sense, we can keep the test
3) does the cljs path roughly contain the namespace where the test failed? It looks like no, and if that's the case then I wouldn't add a test for the cljs side, since it's not a behavior worth keeping

aside) the failing value is represented as a vector because of how prop/for-all works, especially with multiple clauses. If you have multiple clauses in the for-all, you'll see multiple values in the vector

Show
Gary Fredericks added a comment - 1) there are a few macros in test.check itself (defspec, for-all, gen/let) that might serve as examples 2) that makes sense, we can keep the test 3) does the cljs path roughly contain the namespace where the test failed? It looks like no, and if that's the case then I wouldn't add a test for the cljs side, since it's not a behavior worth keeping aside) the failing value is represented as a vector because of how prop/for-all works, especially with multiple clauses. If you have multiple clauses in the for-all, you'll see multiple values in the vector
Hide
Michael Glaesemann added a comment - - edited

Thanks for the feedback and the explanation of the :fail vector.

As discussed, I've added the filename test for :clj only. Not only is there no relevant namespace reference in the :cljs file path, the filename itself is different depending on the output target.

Regarding the additional test, I noticed that my merge in the first patch left a redundant version of the initial can-report-shrinking test. I've removed that initial version. Thanks for bringing it to my attention.

Regarding rewriting the assert-expr defmethod to reduce the number of namespaces, I don't believe it's possible to reduce it to one, at least not for JVM ClojureScript. The reason is that the CLJ and CLJS versions require different clj files – clojure.test and cljs.test, respectively – during the CLJ compilation stage. As far as I know, there's no way to indicate in this CLJ file, require library clojure.test when the ultimate target is CLJ, and cljs.test when it's going to be included in a CLJS file down the road. This is what requires the separate check.clj and check/cljs-macros.clj files. If you know of a method or an implementation that does this, please let me know. I've done quite a bit of re-reading of references on the topic, but I admittedly am still not as strong in my understanding of ClojureScript compilation to be sure I've exhausted the available options.

The check/impl.cljc file is there to keep the code DRY.

I've made a couple of changes which I hope are improvments. I collapsed the check.clj and check.cljs files into a single check.cljc file. I've also renamed the check/cljs_macros.clj file to check/cljs.clj as it is cljs-specific but doesn't include any macro definitions. A file named check/cljs.clj (namespace clojure.test.check.clojure-test-check.cljs is a bit confusing, but the longer alternative check/check_cljs.clj with the namespace clojure.test.check.clojure-test.check.check-cljs doesn't seem to be any better.


For reference, here's a summary of the conditional dependencies reflecting the updated patch:

:clj version dependencies
  • clojure-test.check (.cljc:clj) – clojure.test/assert-expr defmethod
    • clojure-test.check.impl (.cljc/:clj) – check? defn
    • clojure.test (.clj) – clojure.test/assert-expr defmulti
:cljs version dependencies
  • clojure-test.check (.cljc:cljs)
    • clojure-test.check.impl (.cljc:cljs) – check? and check-results cljs defns
    • clojure-test.check.cljs (.clj) – cljs.test/assert-expr defmethod
      • clojure-test.check.impl (.cljc:clj) – check? reference
      • cljs.test (.cljc:clj) – cljs.test/assert-expr defmulti
Show
Michael Glaesemann added a comment - - edited Thanks for the feedback and the explanation of the :fail vector. As discussed, I've added the filename test for :clj only. Not only is there no relevant namespace reference in the :cljs file path, the filename itself is different depending on the output target. Regarding the additional test, I noticed that my merge in the first patch left a redundant version of the initial can-report-shrinking test. I've removed that initial version. Thanks for bringing it to my attention. Regarding rewriting the assert-expr defmethod to reduce the number of namespaces, I don't believe it's possible to reduce it to one, at least not for JVM ClojureScript. The reason is that the CLJ and CLJS versions require different clj files – clojure.test and cljs.test, respectively – during the CLJ compilation stage. As far as I know, there's no way to indicate in this CLJ file, require library clojure.test when the ultimate target is CLJ, and cljs.test when it's going to be included in a CLJS file down the road. This is what requires the separate check.clj and check/cljs-macros.clj files. If you know of a method or an implementation that does this, please let me know. I've done quite a bit of re-reading of references on the topic, but I admittedly am still not as strong in my understanding of ClojureScript compilation to be sure I've exhausted the available options. The check/impl.cljc file is there to keep the code DRY. I've made a couple of changes which I hope are improvments. I collapsed the check.clj and check.cljs files into a single check.cljc file. I've also renamed the check/cljs_macros.clj file to check/cljs.clj as it is cljs-specific but doesn't include any macro definitions. A file named check/cljs.clj (namespace clojure.test.check.clojure-test-check.cljs is a bit confusing, but the longer alternative check/check_cljs.clj with the namespace clojure.test.check.clojure-test.check.check-cljs doesn't seem to be any better.
For reference, here's a summary of the conditional dependencies reflecting the updated patch:
:clj version dependencies
  • clojure-test.check (.cljc:clj) – clojure.test/assert-expr defmethod
    • clojure-test.check.impl (.cljc/:clj) – check? defn
    • clojure.test (.clj) – clojure.test/assert-expr defmulti
:cljs version dependencies
  • clojure-test.check (.cljc:cljs)
    • clojure-test.check.impl (.cljc:cljs) – check? and check-results cljs defns
    • clojure-test.check.cljs (.clj) – cljs.test/assert-expr defmethod
      • clojure-test.check.impl (.cljc:clj) – check? reference
      • cljs.test (.cljc:clj) – cljs.test/assert-expr defmulti
Hide
Gary Fredericks added a comment -

Okay, I'm familiar with the macroexpansion-cljc problem and having a separate namespace seems like the cleanest solution that's likely to exist. It's easy to change later if we think of something better.

The current patch still has three new namespaces, and at a glance I think we could shrink that to two, specifically clojure.test.check.clojure-test.assertions.cljs with the cljs defmethod, and clojure.test.check.clojure-test.assertions with everything else.

In particular I'm trying to avoid the word "check" as it seems too confusing/overloaded to me in the context of this project.

Show
Gary Fredericks added a comment - Okay, I'm familiar with the macroexpansion-cljc problem and having a separate namespace seems like the cleanest solution that's likely to exist. It's easy to change later if we think of something better. The current patch still has three new namespaces, and at a glance I think we could shrink that to two, specifically clojure.test.check.clojure-test.assertions.cljs with the cljs defmethod, and clojure.test.check.clojure-test.assertions with everything else. In particular I'm trying to avoid the word "check" as it seems too confusing/overloaded to me in the context of this project.
Hide
Michael Glaesemann added a comment -

I appreciate your patience and guidance. That works just fine! The naming recommendation is a good one as well.

Show
Michael Glaesemann added a comment - I appreciate your patience and guidance. That works just fine! The naming recommendation is a good one as well.
Hide
Gary Fredericks added a comment -

I applied the patch on master, thanks again. I'm going to look at the javascript line number issue, I wonder if it's just a matter of creating the Error object in the user code.

Show
Gary Fredericks added a comment - I applied the patch on master, thanks again. I'm going to look at the javascript line number issue, I wonder if it's just a matter of creating the Error object in the user code.
Hide
Michael Glaesemann added a comment - - edited

Thanks!

I came across this issue CLJS-1255 when I was working on it originally.

Show
Michael Glaesemann added a comment - - edited Thanks! I came across this issue CLJS-1255 when I was working on it originally.
Hide
Gary Fredericks added a comment -

Here's a proof of concept for getting the line numbers working. It bypasses the file-and-line function in cljs.test and just parses the stack trace by hand, which might be ill-advised. It does seem to work in both node-dev and browser-dev though. It didn't seem to work in node-adv, but I assume compatibility with advanced mode is probably unreasonable anyhow.

Show
Gary Fredericks added a comment - Here's a proof of concept for getting the line numbers working. It bypasses the file-and-line function in cljs.test and just parses the stack trace by hand, which might be ill-advised. It does seem to work in both node-dev and browser-dev though. It didn't seem to work in node-adv, but I assume compatibility with advanced mode is probably unreasonable anyhow.
Hide
Michael Glaesemann added a comment -

I'm finally circling back to this. When I checked out your test branch I get a compile error running lein cljsbuild test.

lein clean && lein cljsbuild test
... reflection warnings omitted
module.js:327
    throw err;
    ^

Error: Cannot find module '../target/cljs/node_dev/out/goog/bootstrap/nodejs.js'
    at Function.Module._resolveFilename (module.js:325:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/Users/grzm/Documents/dev/test.check/resources/run.js:5:1)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
Reloading Clojure file "/clojure/test/check/clojure_test_test.cljc" failed.
clojure.lang.Compiler$CompilerException: java.lang.RuntimeException: No such namespace: js, compiling:(clojure/test/check/clojure_test_test.cljc:58:1)

What am I missing?

Show
Michael Glaesemann added a comment - I'm finally circling back to this. When I checked out your test branch I get a compile error running lein cljsbuild test.
lein clean && lein cljsbuild test
... reflection warnings omitted
module.js:327
    throw err;
    ^

Error: Cannot find module '../target/cljs/node_dev/out/goog/bootstrap/nodejs.js'
    at Function.Module._resolveFilename (module.js:325:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/Users/grzm/Documents/dev/test.check/resources/run.js:5:1)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
Reloading Clojure file "/clojure/test/check/clojure_test_test.cljc" failed.
clojure.lang.Compiler$CompilerException: java.lang.RuntimeException: No such namespace: js, compiling:(clojure/test/check/clojure_test_test.cljc:58:1)
What am I missing?
Hide
Gary Fredericks added a comment -

I will look closer at this shortly, but in the meantime you could check this branch, which I think is more baked than the first commit I pointed to.

Show
Gary Fredericks added a comment - I will look closer at this shortly, but in the meantime you could check this branch, which I think is more baked than the first commit I pointed to.
Hide
Gary Fredericks added a comment -

Yeah, the TCHECK-34-js-file-and-line-2 branch seems to work well. Sorry for the earlier broken stuff (I suspect it probably works in cljs only as long as you do lein cljsbuild once or something like that).

Show
Gary Fredericks added a comment - Yeah, the TCHECK-34-js-file-and-line-2 branch seems to work well. Sorry for the earlier broken stuff (I suspect it probably works in cljs only as long as you do lein cljsbuild once or something like that).

People

Vote (1)
Watch (3)

Dates

  • Created:
    Updated: