Implement quickcheck::Arbitrary for IPLD type for testing

[connormullett]

ChainSafe's project Forest recently added property testing for serialization/deserialization of datatypes after finding a typo one day. We've been using quickcheck and need to implement Arbitrary on some of these types. In order to clean up our codebase from using Ipld wrappers, we'd like to push them upstream into the respective repositories.

The tracking issue can be found here

Thanks!

[lemmih]

A few changes are needed:

• Putting the implementation behind a #[cfg(test)] guard means it won't be usable outside this crate. The implementation literally only exists when libipld-core is being tested.
• The implementation needs to live behind a feature flag.
• quickcheck_macros is not used.
• The generated lists and maps are always empty. They should be populated recursively while respecting Gen.size.
• rust-cid will soon support quickcheck-1. There should be a note in the code saying that Cid::arbitrary(g) should be used when it is available.

[rkuhn]

Thanks for pushing this! Quickcheck tests can be a real life-saver, or a productivity boost, at least for me — but this second part crucially depends on implementing shrink to quickly find the culprit. Would you be up for adding that? (This is mainly necessary for variably sized inputs that can be large, in this case List and Map (and possibly String and Bytes; my implementation recipe is to iterate over all ways in which the input can be made “smaller by one”).

[connormullett]

Sure! I'll see what I can do

[connormullett]

I've been spinning my wheels on this for a while now and haven't made much progress at all. I believe the conflict is because of the signature in shrink and IPLD being an enum with IpldIter being a separate type. I can't figure it out and the documentation basically doesn't exist. I understand why it would be great to have (and the docs do mention how useful it is) but I can't find anything to help implement this

[connormullett]

The CI workflow error looks to be not on my side, the github action doesn't have the wasm32 target

[rkuhn]

I’d try it with something like the following:

    fn shrink(&self) -> Box<dyn Iterator<Item = Self>> {
        match self {
            Ipld::List(list) => {
                let list = list.clone();
                let first = list.get(0).cloned();
                Box::new(
                    (0..list.len())
                        .map(move |remove| {
                            Ipld::List(
                                list.iter()
                                    .enumerate()
                                    .filter_map(|(idx, ipld)| {
                                        if idx == remove {
                                            None
                                        } else {
                                            Some(ipld.clone())
                                        }
                                    })
                                    .collect(),
                            )
                        })
                        .chain(
                            first
                                .map(|ipld| ipld.shrink())
                                .unwrap_or_else(|| Box::new(std::iter::empty())),
                        ),
                )
            }
            Ipld::Map(_) => todo!(),
            _ => Box::new(std::iter::empty()),
        }
    }

As far as I understand quickcheck, we only need to shrink by one step at a time, we don’t need an iterator that produces all shrunk versions in one go.

[vmx]

The reason why CI for WASM fails is due to quickcheck drawing in getrandom. A potential upstream fix is at BurntSushi/quickcheck#275.

One way is adding the getrandom/js feature here. It could be done through a wasm feature, or matching on the wasm arch (I guess). But I'm not sure if that's really needed/desierd. Hence I propose to just skip the quickcheck feature on the CI when it is compiled for wasm (and list all other features manualy).

[vmx]

I wonder if it would make sense to follow rust-cid and rust-multihash and name this feature arb instead. This way it could potentially also provide an implementation for https://crates.io/crates/arbitrary.

Related: would it make sense to remove the dep:? It would make the library need a quite new rust stable version, and I prefer bumping it only when there's a good reason to.

[lemmih]

PR #167 will incorporate the comments and suggestions from this PR.

[lemmih]

This PR can be closed.