BurntSushi / quickcheck

Automated property based testing for Rust (with shrinking).
The Unlicense
2.4k stars 149 forks source link

Consider implementing Testable for Option<bool> #105

Closed carllerche closed 5 years ago

carllerche commented 8 years ago

It seems like Option<bool> would be a simple way to discard tests vs. having to return a TestResult directly.

BurntSushi commented 8 years ago

I personally kind of like writing TestResult::discard because it's more explicit what's going on. I don't feel terribly strongly though.

porglezomp commented 8 years ago

I think TestResult is much clearer, and it's not very inconvenient in my opinion.

andrewhickman commented 6 years ago

This would be nice for use with the checked_ functions e.g.

#[quickcheck]
fn test_div_rem(a: i64, b: i64) -> Option<bool> {
    let div = a.checked_div(b)?;
    let rem = a.checked_rem(b)?;
    Some(div * b + rem == a)
}
tobz1000 commented 5 years ago

Has this been reconsidered? I'd be happy to submit a PR.

The more ideal solution would probably be to implement the Try trait for TestResult, allowing for early return of TestResult::discard(), but it doesn't look like the trait will stabilise any time soon.

BurntSushi commented 5 years ago

I remain unconvinced that this is worth doing. I'd rather see tests being explicitly discarded.

neithernut commented 3 years ago

Types like the one drafted in #281 (whether they are part of this library or implemented on the user's side) create use-cases where returning an Option would be preferable to the alternatives imo. Consider something like the following:

fn revrev(original: u128) -> Option<Equivalence<u128>> {
    if original == u128::MAX {
        // for some reason, u128::MAX must be ignored for the test
        None
    } else {
        // pseudo idendity
        Some(Equivalence::of(xs, revrev))
    }
}

Without an Testable implementation for Option, we'd need to write something like:

fn revrev(original: u128) -> TestResult {
    if original == u128::MAX {
        // for some reason, u128::MAX must be ignored for the test
        TestResult::discard()
    } else {
        // pseudo idendity
        Equivalence::of(xs, revrev).into_result(Gen::new(0))
    }
}

... with the very unsightly Gen::new(0) which shouldn't even used in the conversion, if it is a conversion. That is, unless there is some other way to convert Equivalence into a TestResult. Such a function could be mandated for such convenience types, but it would require manually writing such a conversion for all those types (or introduce a trait explicitly for such conversions and maybe impl Testable via that trait for the conversion case).

Of course, discarding a test/value explicitly would be far more expressive. And I myself am not sure whether using an Option is really intuitive or whether it would confuse some users. Maybe a type expressing this intend explicitly would be worth considering?