Closed nberger closed 8 years ago
There was a comment in the original .clojure-test
code that we were conflating errors and failures somehow -- is that what this test is about, and would it therefore fail on the older versions of .clojure-test
as well? I'm just clarifying that this isn't a regression.
I haven't looked at the code yet, but wanted to mention that we'll need to think about the difference between clojure.test's notion of code failing in an assertion vs. outside it. I.e., the following expressions are reported differently (and the code for #25 also treats them (accidentally I think) differently):
(is (/ 42 n))
(let [x (/ 42 n)]
(is x))
There was a comment in the original .clojure-test code that we were conflating errors and failures somehow -- is that what this test is about, and would it therefore fail on the older versions of .clojure-test as well? I'm just clarifying that this isn't a regression.
Maybe :)
My understanding is that an error is something that prevented a test to execute fully, like an exception not in an assertion, and a failure is an assertion that didn't hold true. Given that basic definition, the idea of this PR is to add a test to verify that we report failures correctly. We already have a test to verify that exceptions are reported as errors (even though we know it's broken, but that's another story), but we didn't have a test to verify that failures are reported as failures... Here's the test, we see it fails, and my conclusion is that #21 not only broke the feature of "reporting exceptions as errors" but also (probably more important) the feature of "reporting failures as failures".
Would be great if you can track down that comment in clojure.test
... Also what do you mean by "older version of clojure.test", which version exactly?
we'll need to think about the difference between clojure.test's notion of code failing in an assertion vs. outside it. I.e., the following expressions are reported differently (and the code for #25 also treats them (accidentally I think) differently)
Yes, I agree, this is critical. My assumption is that we need to mimic clojure.test behavior:
What do you think?
Sorry to be confusing; I'm just talking about this comment, currently on master.
When I mentioned .clojure-test
I was talking about com.gfredericks.test.chuck.clojure-test
, not about clojure.test
.
I believe the statistics that clojure.test
uses count :fail
as an assertion that is falsy, while :error
is an exception caught either within an assertion or outside of it. Only the latter causes the test to stop executing, I think.
Oh, sorry I missed the dot in .clojure-test
. That's clear now.
I believe the statistics that clojure.test uses count :fail as an assertion that is falsy, while :error is an exception caught either within an assertion or outside of it. Only the latter causes the test to stop executing, I think.
Hmmm, yes, it's probably as you are saying. I'll try to check it in clojure.test
code.
.clojure-test
should mimic clojure.test.check.clojure-test
behavior, right? clojure.test.check.clojure-test
reports an exception as an :error, and a falsy assertion as :fail. There is a test verifying the falsy assertion behavior. There is no test (in test.check) about the exception reported as error, I wrote one in https://github.com/nberger/test.check/blob/add-error-checking-test/src/test/clojure/clojure/test/check/clojure_test_test.clj#L81-L94. I think I'll send it as a patch to test.check :)
Yeah the closer we can get to clojure.test
behavior the better.
Rebased to master and updated the failure-detection-test
because the test report shows 1 passed and 1 failed test, when I think there should just 1 failed.
Perhaps I'm missing something... Anyways, I still think this new test has its value in verifying that failures are being reported. Left a comment as a TODO to check that :pass 1
Looks good, thanks!
The test is currently failing, but I think we have to add a test like this not only to ensure that we detect & report exceptions correctly (as reported in #23) but also that we do the same for failures.