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

Switch to quickcheck 1.0 #9

Merged
merged 2 commits into from
Aug 25, 2023
Merged

Switch to quickcheck 1.0 #9

merged 2 commits into from
Aug 25, 2023

Conversation

ziutech
Copy link
Contributor

@ziutech ziutech commented Aug 6, 2023

As far as I understand the only breaking change was published in this commit.

I found two significant differences:

  1. signature for Arbitrary::arbitrary changed
  2. it's no longer possible to call RngCore::next_u32

I changed every call to RngCore::next_u32 to u32::arbitrary. It was based purely on intuition, and seems to be wrong. The tests build, but don't pass — somewhere it tries to allocate more memory than max of u32. As I am writing this I'm not sure how to solve this. I would have to get more insight into how quickcheck works and why the tests are implemented the way they are.

@danjl1100
Copy link

danjl1100 commented Aug 7, 2023

Looking at this from the stream's mention of #8, the failing test seems to be the with_cap function in tests/quick.rs

fn with_cap(cap: usize) -> bool {
    let vs: Vc<u8> = Vc::with_capacity(cap); // <-- attempts to allocate for arbitrary `usize`
    println!("wish: {}, got: {} (diff: {})", cap, vs.capacity(), vs.capacity() as isize - cap as isize);
    vs.capacity() >= cap
}

It seems like before this PR, in quickcheck 9.2, the only cap sizes tested were powers of 2, ending below 256. Unclear to me where this upper bound was coming from.
Note: I found this by adding a panic before the allocation if the size is too large (println/eprintln did not seem to come through?).
I understand quickcheck is RNG-driven, but I'm not seeing any fails before this PR with this diagnostic added:

 fn with_cap(cap: usize) -> bool {
+    if cap > 256 {
+        panic!("{}", format!("about to test cap {cap}...")); // Rust pre-2021 edition feels odd :)
+    }
     let vs: Vc<u8> = Vc::with_capacity(cap);
     println!("wish: {}, got: {} (diff: {})", cap, vs.capacity(), vs.capacity() as isize - cap as isize);
     vs.capacity() >= cap
 }

But now in quickcheck >=1.0, the sizes tested are more what we would expect for an arbitrary usize. The test aborts when trying to allocate ~100GB (more than local RAM), before quickcheck gets to try finding a miminal failing size.

Echoing @ziutech's sentiment, I am not sure what range of input this specific Vc::with_capacity test is meant to accept.

@d2weber
Copy link

d2weber commented Aug 7, 2023

The behaviour of quickcheck changed in a way, that the size parameter does not anymore limit the range of the generated values. It now is only used for the number of values generated. The default size is 100.
BurntSushi/quickcheck@71d743a

Edit: there is an unresolved issue asking for a way to specify the generated range: BurntSushi/quickcheck#267

@d2weber d2weber mentioned this pull request Aug 17, 2023
@jonhoo
Copy link
Owner

jonhoo commented Aug 20, 2023

Ah, that test is arguably a little silly, but I would do something like use a u8::arbitrary(g) and then just cast it to usize. That tests as much as we care about for that call I believe.

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great now, thanks!

@jonhoo jonhoo merged commit ff32dc3 into jonhoo:main Aug 25, 2023
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

Successfully merging this pull request may close these issues.

4 participants