Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add back a way to put a bound on numbers generated #267

Open
BurntSushi opened this issue Jan 8, 2021 · 14 comments
Open

add back a way to put a bound on numbers generated #267

BurntSushi opened this issue Jan 8, 2021 · 14 comments
Assignees

Comments

@BurntSushi
Copy link
Owner

It turns out that the tests inside byteorder were designed around the ability to control the bounds of integers generated and there is no easy way to adapt them to quickcheck 1.0 without this ability. I think we can just add a bound mutator on Gen that is by default not set.

@dmit
Copy link

dmit commented Jan 8, 2021

Would this bring back gen_range for the new Gen struct, or offer an equivalently powerful solution?

Previously the Gen trait inherited gen_range from the underlying rand::Rng, but now that it's a struct its gen and gen_range methods are private. Would it be possible to make them public? The usefulness of gen_range in particular is evident given how many of the provided Arbitrary impls for std types utilize it.

@BurntSushi BurntSushi self-assigned this Jan 8, 2021
@BurntSushi
Copy link
Owner Author

The intent is to add back methods yes. But I'm not making rand a public dependency.

However, this issue is about something different. This is a configuration knob on Gen itself.

If you would like more methods on Gen, please file new issues with the desired method signatures.

@dmit
Copy link

dmit commented Jan 8, 2021

As far as I can tell, making existing private methods on Gen public doesn't expose rand as a dependency. They mirror rand's signatures, but don't leak any implementation details.

But will do!

@BurntSushi
Copy link
Owner Author

It does expose rand. Their type signatures include things from the rand crate.

@jhpratt
Copy link
Contributor

jhpratt commented Jan 9, 2021

Looking through upgrading to 1.0, and this would be great to have.

Presumably it would be possible to accept impl RangeBounds as a parameter in the same way rand does? That wouldn't expose any implementation details, but would allow for using a similar API.

@dvc94ch
Copy link

dvc94ch commented Jan 19, 2021

quickcheck seems severely limited without gen_range and gen_bool.

@BurntSushi
Copy link
Owner Author

You don't need gen_bool. Arbitrary::bool should work just fine.

It would probably be good to add gen_range, but it doesn't seem particularly difficult to use modulus instead.

@BurntSushi
Copy link
Owner Author

BurntSushi commented Jan 19, 2021

And once again, this issue is unrelated to adding helper methods in Gen. This is about bounding values generated by Arbitrary impls for number types.

vmx added a commit to multiformats/rust-multihash that referenced this issue Feb 5, 2021
We now have dependabot, but as there are several outdated packages,
I thought I upgrade the simple ones once more manually.

`quickcheck` (and `rand`) are likely not upgraded until there is a good way
to generate ranges (see BurntSushi/quickcheck#267
for more information).

Also `parity-scale-codec` needs more work.
vmx added a commit to multiformats/rust-multihash that referenced this issue Feb 5, 2021
We now have dependabot, but as there are several outdated packages,
I thought I upgrade the simple ones once more manually.

`quickcheck` (and `rand`) are likely not upgraded until there is a good way
to generate ranges (see BurntSushi/quickcheck#267
for more information).

Also `parity-scale-codec` needs more work.
@sdroege
Copy link

sdroege commented Feb 13, 2021

I also need this to update some of my code to quickcheck 1.0, specifically to generate floating point numbers in a specific range.

@Ralith
Copy link

Ralith commented Mar 1, 2021

It would probably be good to add gen_range, but it doesn't seem particularly difficult to use modulus instead.

Floating point values don't have such a convenient hack, unfortunately.

dfoxfranke added a commit to akamai-contrib/byztimed that referenced this issue Mar 10, 2021
The following obsolete dendencies remain:

* quickcheck 0.9 (current: 1.0): Byztime's tests depend on some removed functionality (see BurntSushi/quickcheck#267) and I can't be bothered to deal with this right now.

* rand 0.7 (current: 0.8): required by quickcheck 0.9.

* aead 0.3 (current: 0.4): required by aes_siv 0.5.
@ghost
Copy link

ghost commented May 22, 2021

It would probably be good to add gen_range, but it doesn't seem particularly difficult to use modulus instead.

Modulus breaks shrinking. QuickCheck shrinks the generated number, but your number after % can go from MIN_VALUE to MAX_VALUE. In such case, shrinking the number creates a bigger number. This is not at all helpful or useful.

As an example, the values generated in this manner look like this in failures:

assertion failed: `(left == right)`
  left: `(8, 4, 32, 6)`,
 right: `(8, 4, 32, 8)`

The actual failing case, after shrinking, would be either (2, 0, 0, 2) or (1, 59, 59, 29). Other QuickCheck implementations that allow controlling bounds find these cases immediately.

Modulus is completely wrong for this purpose.

@BurntSushi
Copy link
Owner Author

@adam-becker I don't know what you're talking about. I was of course referring to the implementation of Arbitrary::arbitrary, not shrinking. This issue has nothing to do with shrinking, which doesn't even get access to a Gen:

quickcheck/src/arbitrary.rs

Lines 732 to 768 in defde6f

macro_rules! unsigned_shrinker {
($ty:ty) => {
mod shrinker {
pub struct UnsignedShrinker {
x: $ty,
i: $ty,
}
impl UnsignedShrinker {
pub fn new(x: $ty) -> Box<dyn Iterator<Item = $ty>> {
if x == 0 {
super::empty_shrinker()
} else {
Box::new(
vec![0]
.into_iter()
.chain(UnsignedShrinker { x: x, i: x / 2 }),
)
}
}
}
impl Iterator for UnsignedShrinker {
type Item = $ty;
fn next(&mut self) -> Option<$ty> {
if self.x - self.i < self.x {
let result = Some(self.x - self.i);
self.i = self.i / 2;
result
} else {
None
}
}
}
}
};
}

@justinlovinger
Copy link

justinlovinger commented Oct 23, 2022

What is the status of this issue? The number of issues from other projects referencing this one is a clear sign people want this resolved.

P.S. A quick check (no pun intended) shows at least one project implemented this functionality on top of QuickCheck, libp2p/rust-libp2p#2857, https://github.com/libp2p/rust-libp2p/blob/master/misc/quickcheck-ext/src/lib.rs. However, I am not sure their implementation is sound when shrinking, and it only works for unsigned numbers.

@Colossus

This comment was marked as duplicate.

dead-claudia added a commit to dead-claudia/journald-exporter that referenced this issue Jul 6, 2024
...and switch the 32-bit integer parser to just exhaustive checking.
(More on that later.)

Why move away from QuickCheck?

1. The maintainer appears to have little interest in actually
   maintaining it. BurntSushi/quickcheck#315

2. Its API is incredibly inefficient, especially on failure, and it's
   far too rigid for my needs. For one, I need something looser than
   `Arbitrary: Clone` so things like `std::io::Error` can be generated
   more easily. Also, with larger structures, efficiency will directly
   correlate to faster test runs. Also, I've run into the limitations
   of not being able to access the underlying random number generator
   far too many times to count, as I frequently need to generate random
   values within ranges, among other things.
   - BurntSushi/quickcheck#279
   - BurntSushi/quickcheck#312
   - BurntSushi/quickcheck#320
   - BurntSushi/quickcheck#267

3. It correctly limits generated `Vec` and `String` length, but it
   doesn't similarly enforce limits on test length.

4. There's numerous open issues in it that I've addressed, in some
   cases by better core design. To name a few particularly bad ones:
   - Misuse of runtime bounds in `Duration` generation, `SystemTime`
     generation able to panic for unrelated reasons:
     BurntSushi/quickcheck#321
   - Incorrect generation of `SystemTime`:
     BurntSushi/quickcheck#321
   - Unbounded float shrinkers:
     BurntSushi/quickcheck#295
   - Avoiding pointless debug string building:
     BurntSushi/quickcheck#303
   - Signed shrinker shrinks to the most negative value, leading to
     occasional internal panics:
     BurntSushi/quickcheck#301

There's still some room for improvement, like switching away from a
recursive loop: BurntSushi/quickcheck#285.
But, this is good enough for my use cases right now. And this code
base is structured such that such a change is *much* easier to do.
(It's also considerably simpler.)

As for the integer parser change, I found a way to re-structure it so
I could perform true exhaustive testing on it. Every code path has
every combination of inputs tested, except for memory space as a whole.
This gives me enough confidence that I can ditch the randomized
property checking for it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants