Support quickcheck based property testing

[cmichi]

I'm open for other approaches to solve this.

The problem we have with the new env rev 3 is that quickcheck tests don't run, since quickcheck runs multiple tests in one thread whereas our testing environment expects one test per thread (because of thread_local!).

With multiple tests being subsequently run in the same thread by quickcheck we get an AlreadyInitialized assertion error.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@82c9629). Click here to learn what that means.
The diff coverage is 98.57%.

@@            Coverage Diff            @@
##             master     #319   +/-   ##
=========================================
  Coverage          ?   64.29%           
=========================================
  Files             ?       70           
  Lines             ?     5252           
  Branches          ?        0           
=========================================
  Hits              ?     3377           
  Misses            ?     1875           
  Partials          ?        0

Impacted Files	Coverage Δ
core/src/lib.rs	100% <ø> (ø)
core/src/env/engine/off_chain/test_api.rs	70.37% <100%> (ø)
core/src/storage/collections/hash_map/tests.rs	98.26% <98.3%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82c9629...927218d. Read the comment docs.

[cmichi]

@Robbepop Thanks for the comments! I implemented it in a bit of a different way now. Could you take another look please?

[Robbepop]

I think I see how you try to fix the problems with quickcheck running tests multi threaded, however, to me it currently seems that this causes race conditions onto the underlying off-chain environment that has not at all been implemented in a thread safe manner.

To summarize:

• quickcheck runs multiple tests in a single thread
• The ink! off-chain environment has a single instance per thread which is no problem for normal Cargo-based tests that run each in their own thread.
• The error we see when quickcheck tests are run is that an already initialized off-chain environment is initialized again.
• This PR tries to fix this particular error by introducing a laxer initialization scheme for the off-chain environment.

This fix is problematic since then multiple tests are accessing the off-chain environment concurrently for which it has never been designed. Also the tests might override each others off-chain instances which is death to debugging.
I hope I understood this whole thing correctly. Please correct me if I am wrong.

As of now I would not try to dig further with this approach and instead try to force quickcheck to be running in their own thread as Cargo enforces it for its tests. I do not see any other "easy" solution that would not require us to change the entire functionality of the off-chain instance in order to make it concurrent.

[Robbepop]

With multiple tests being subsequently run in the same thread by quickcheck we get an AlreadyInitialized assertion error.

If tests run consecutively this should have been fixed in my upcoming PR for storage2 at least. We could port this fix potentially to storage (revision 1) if needed.

[codecov-commenter]

Codecov Report

Merging #319 into master will increase coverage by 0.75%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   86.67%   87.42%   +0.75%     
==========================================
  Files         107      108       +1     
  Lines        4219     4272      +53     
==========================================
+ Hits         3657     3735      +78     
+ Misses        562      537      -25     

Impacted Files	Coverage Δ
core/src/storage2/collections/hashmap/mod.rs	98.18% <ø> (ø)
...ore/src/storage2/collections/hashmap/fuzz_tests.rs	100.00% <100.00%> (ø)
core/src/env/engine/off_chain/db/accounts.rs	81.69% <0.00%> (+2.81%)	⬆️
core/src/env/engine/off_chain/db/console.rs	26.31% <0.00%> (+10.52%)	⬆️
core/src/env/engine/off_chain/mod.rs	89.47% <0.00%> (+13.15%)	⬆️
core/src/env/engine/off_chain/db/chain_spec.rs	100.00% <0.00%> (+23.80%)	⬆️
core/src/env/engine/off_chain/db/events.rs	71.42% <0.00%> (+28.57%)	⬆️
core/src/env/engine/off_chain/runtime_storage.rs	100.00% <0.00%> (+40.00%)	⬆️
core/src/env/engine/off_chain/runtime_calls.rs	100.00% <0.00%> (+50.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65e094e...1897f43. Read the comment docs.

[cmichi]

Just updated the PR and it can be reviewed again.

I was able to remove the bulk of the original work, which was originally necessary because of the issue with offchain environments not being reset and quickcheck running in single thread mode. This has resolved itself through other changes in the meantime.

So this PR now "just" contains some fuzz tests for storage2/HashMap :-).

In order to execute all tests including fuzz tests use cargo test --features ink-fuzz. In order to just execute the ones from this PR use cargo test --features ink-fuzz -- randomized_. Imho a dedicated prefix for fuzz tests is a good practice (we could also use fuzz_ instead of randomized_, I'm open to that).

[cmichi]

Updated! Execute all fuzz tests via cargo test --features ink-fuzz-tests -- fuzz_tests.

[Robbepop]

LGTM besides some nits here and there.
Can you provide us with overall results of your fuzz tests?
Have they found bugs? I myself am unexperienced with fuzz testing so would appreciate more information or another review by someone who has more knowledge than I do.

Also have you looked into proptest? -> https://altsysrq.github.io/proptest-book/proptest/vs-quickcheck.html
To be fair the linked article is biased towards proptest but still to me it looks a bit more flexible. We should definitely not use multiple fuzz testers if we ever want to expand on our fuzz testing capabilities. Also note that I hereby do not propose to change this PR into using proptest just for the sake of it. We can still do this in a follow-up.

[cmichi]

Can you provide us with overall results of your fuzz tests? Have they found bugs?

I used it heavily while implementing the BTreeMap and found a number of bugs there. Those were mostly edge cases which I hadn't considered for unit tests beforehand, I then added tests for those cases. The fuzz tests for BTreeMap look very similar to the ones for this data structure and I can provide a follow-up PR at some point (maybe also for other data structures, it's always similar, a pattern of insert/removes ops). I would wait until we have agreed on a fuzzing library though.

The fuzz tests were left out of the original BTreeMap PR since that PR already introduced a lot of new stuff, instead I had created this separate PR for the already existing HashMap at that time.

Also have you looked into proptest?

As discussed in DM I have created a follow-up issue to evaluate and migrate to a better fuzzing library (#464).

[Robbepop]

still for_each used instead of normal loop