search

reiddraper / simple-check

QuickCheck for Clojure
http://reiddraper.github.io/simple-check/
286 stars 18 forks source link

`choose` excludes its upper bound #54

Closed hypirion closed 10 years ago

hypirion commented 10 years ago

The formula for calculating the next random int in rand-range is implemented as follows:

(let [factor (.nextDouble rnd)]
  (long (Math/floor (+ lower (- (* factor upper) (* factor lower))))))

This is the same implementation as data.generators/uniform, which states that the upper bound is excluded. However, simple-check.generators/choose, which uses this internally, states that the upper bound is included.

This error bubbles down in all generators which uses choose internally: the generator created by elements will never return the last element of the input collection, one-of will never generate values from the last generator, boolean will always return false, and so on.

The easiest solution to solve this issue is by incrementing the upper bound with 1.0 (1.0 to avoid potential long overflows) in the previously mentioned snipped:

(let [factor (.nextDouble rnd)]
  (long (Math/floor (+ lower (- (* factor (+ 1.0 upper)) (* factor lower))))))

(I would've sent a pull request, if it weren't for the lockdown which I assume is still in place?)

cemerick commented 10 years ago

Ouch. That's my bad, via #37 and the associated PR. Really good catch; egg on my face for sure, esp. since I've been using that faulty rand-range impl for months. :-(

I don't know about the "lockdown", etc., but this is a pretty big deal. I'm happy to produce a PR (maybe including some regression tests that watch for stuff like this, at a minimum verifying that e.g. all of the values provided to elements are returned after some very large number of samples?), but I'd certainly understand if https://github.com/reiddraper/simple-check/commit/b39a1686ddf0f70fd704ebe2cdf01187cabe67e7 is reverted first.

@reiddraper?

reiddraper commented 10 years ago

I'd love a fix. The 'lockdown' I've described is just for new contributors (signed CA or not), as the clojure-dev mailing list thread specifically names a contributors list. My understanding was that it would not be sufficient to just have new contributors reply to that thread, without having everyone else have to update their post with the new name added. Maybe I'm wrong?