search

marick / Midje

Midje provides a migration path from clojure.test to a more flexible, readable, abstract, and gracious style of testing
MIT License
1.69k stars 129 forks source link

Stop treating exceptions in for-all as passing tests #445

Closed AdamFrey closed 6 years ago

AdamFrey commented 6 years ago

Fix for-all tests that only work for hard-coded seed

There were two tests in the t_for_all tests that shouldn't pass logically, but happen to pass for the given seed. I believe this was done in error, so I removed the logical errors from the tests

Stop swallowing exceptions in for-all fact setup

Previously when an exception occured in a for-all fact but not in a checkable, the exception was silently swallowed and the fact returned true. This happened because the generative form was checking against the truthiness of the value in the quick-check response under the key :result. But when an exception happened the :result was an exception object, not false so generative midje form proceeded as if the test had passed.

In this commit I've updated the test.check version to 0.10.0-alpha3 because a :pass? key has been added to the quick-check response map which we can use more accurately check the run state. Here is the commit where the :pass? key was added: clojure/test.check@09927b6

The test.check version we were using (0.9.0) was released in Nov 2015. The new 0.10.0-alpha3 was released May 2018. Here is the changelog for test.check: https://github.com/clojure/test.check/blob/master/CHANGELOG.markdown

I believe that its beneficial to stay up to date with changes in test.check, but I can understand if people are wary of depending on an alpha version of test.check, and I could find a different way to check for fix this problem without updating the dependency if it needs to be done.

AdamFrey commented 6 years ago

I replaced the hardcoded :seed values with a :num-tests 100 to be more sure that we will get the failing tests we are expecting.

AdamFrey commented 6 years ago

Actually thinking about this, I don't believe there's a risk to making the :num-tests higher for the tests that are written to fail. I'm going to bump the number to 1000, to make the chance of having a unlucky sequence of numbers sneak in there lower

philomates commented 6 years ago

just pushed a release to clojars: https://clojars.org/midje/versions/1.9.3-alpha1