Introduce proptest in chainstate #294

azarovh · 2022-07-20T08:47:17Z

Replace explicit random number generation in chainstate tests with proptest.

This crate generates input values automatically. In case of a test failure it performs input shrinking (the process of finding minimal test input to reproduce the fail). After that it store the seed of failed case in a file for future regressions.

Example of regression seeds:

# Seeds for failure cases proptest has generated in the past. It is
# automatically read and these particular cases re-run before any
# novel cases are generated.
#
# It is recommended to check this file in to source control so that
# everyone who runs the test benefits from these saved cases.
cc 9063b1363c7586876acf0491e5ce091fa9b32487f615fe1dd269664af493c862 # shrinks to atoms1 = 101461, atoms2 = 1064

By default every test is run 256 times with different inputs. But for the cases in chainstate it's an overhead, so I configured proptest to run each test once.

P.S. while reading the code I noticed that chainstate/src/lib.rs exposes ChainstateInterfaceImpl and detail::Chainstate as public, which looks wrong considering there is dedicated public interface ChainstateInterface.

iljakuklic

Looks good. Just some small nitpicks.

chainstate/src/detail/mod.rs

chainstate/Cargo.toml

stanislav-tkach

👍

chainstate/src/detail/tests/double_spend_tests.rs

chainstate/src/detail/tests/events_tests.rs

chainstate/src/detail/tests/processing_tests.rs

chainstate/src/detail/tests/syncing_tests.rs

TheQuantumPhysicist

Besides the naming, I think everything looks fine.

chainstate/src/detail/tests/mod.rs

iljakuklic

Added some comments. The one on TestFramework::random_block I consider quite important to address (to be discussed whether now or as a subsequent PR)

chainstate-storage/src/mock.rs

chainstate/src/detail/tests/mod.rs

chainstate/src/detail/tests/test_framework.rs

chainstate/src/detail/mod.rs

chainstate/src/lib.rs

test-utils/Cargo.toml

iljakuklic · 2022-07-25T14:47:17Z

Was experimenting a bit with ways we could pass custom or randomly-initialized seeds to test cases, and the following hacekd together prototype seemed to work okay:

use test_case::test_case;
use rand::{rngs::StdRng, Rng, SeedableRng, RngCore};
type Seed = <StdRng as SeedableRng>::Seed;

// The test previously failed with these seeds. Fixed in commit 0xabcd, blah blah.
#[test_case([0x12; 32])]
#[test_case([0x13; 32])]
// Generate a new seed at random.
#[test_case(rand::thread_rng().gen())]
fn silly(seed: Seed) {
    let mut rng = StdRng::from_seed(seed);
    let x = rng.next_u32() as u64;
    assert_eq!(2 * x, x + x)
}

It uses the test_case crate to run the test with custom seeds (presumably populated with seeds that failed at some point) followed by test case that generates a single fresh seed. There are other crates with similar functionality (e.g. rstest, parametrized); test_case seemed to work fine but there might be better options.

Note this prototype uses rand directly, which the real version should not, so really the annotations would look something like:

#[test_case(Seed::from_str("1212121212121212..."))]
#[test_case(Seed::from_str("1313131313131313..."))]
#[test_case(Seed::fresh())]

If the Seed::fresh constructor prints the random seed to stdout when it's run, it will show up in the output upon test failure.

Thoughts / comments?

TheQuantumPhysicist · 2022-07-25T19:27:11Z

If this prints the seeded value, then I think it's a good choice.

azarovh · 2022-07-25T19:50:15Z

Thoughts / comments?

I like the idea and will try to apply it. rstest looks like the preferred option here because it will give us fixture functionality as well.

Sam, I believe that seed will be printed with the same function as right now (or maybe in fixture, not sure yet). The difference is that a seed would be a parameter to test case and the regression list would be explicit.

iljakuklic · 2022-07-26T07:40:21Z

Sorry closed the PR by accident

iljakuklic · 2022-07-26T07:47:18Z

If this prints the seeded value, then I think it's a good choice.

Well there are two options:

We print the seed to stdout or stderr when it's generated. It will then be shown in the test output (which is only printed when the test fails by default)
Some of the crates I mentioned support printing the function inputs, e.g. rstest has the #[trace] attribute that enables it.

I am leaning towards (1) since it gives more freedom to pick these crates. It also only prints the seed, in case the table driven testing is used for more stuff besides the seed (i.e. there are multiple arguments, only one of them is seed, we probably want to only print the seed, not several kB of block data). However, there might be other criteria that I have not considered.

TheQuantumPhysicist · 2022-07-26T07:50:25Z

Let's start with simple printing, since Stanislav showed that nextest also prints things correctly and doesn't mix output from different threads.

iljakuklic · 2022-07-26T09:09:11Z

Thoughts / comments?

I like the idea and will try to apply it. rstest looks like the preferred option here because it will give us fixture functionality as well.

The rstest crate seems more featureful and can be made more ergonomic to with automatic conversions, which is a clear advantage of that crate.

However, there are some other criteria that come to my mind that should be considered when picking the crate to use:

I noticed test_case generates a Rust test for each test case (with naming scheme: test_name + :: + test_case_name). That means the test cases show up individually in the listing of tests and can be run/filtered individually. It would be nice to have that feature, not sure how other crates fare in this regard.
In a security-sensitive application, the dependency footprint should also be considered. Pulling more third-party code increases the attack surface. Maybe this is not as much of an issue in testing but still should be given some weight.

Please be aware that I only used test_case and only very briefly, so my comments come largely from a place of ignorance.

azarovh · 2022-07-26T10:05:16Z

rstest with #[trace] attributte produce nice structured output that doesn't require us to write any additional macros.

cargo test --package chainstate --lib -- detail::tests::double_spend_tests

     Finished test [unoptimized + debuginfo] target(s) in 0.28s`
     Running unittests src/lib.rs (target/debug/deps/chainstate-c420b73216d9337b)

running 5 tests
test detail::tests::double_spend_tests::spend_output_in_the_same_block_invalid_order::case_1 ... ok
test detail::tests::double_spend_tests::spend_bigger_output_in_the_same_block::case_1 ... FAILED
test detail::tests::double_spend_tests::double_spend_tx_in_another_block::case_1 ... ok
test detail::tests::double_spend_tests::double_spend_tx_in_the_same_block::case_1 ... ok
test detail::tests::double_spend_tests::spend_output_in_the_same_block::case_1 ... FAILED

failures:

---- detail::tests::double_spend_tests::spend_bigger_output_in_the_same_block::case_1 stdout ----
------------ TEST ARGUMENTS ------------
seed = Seed(16783609377823135750)
-------------- TEST START --------------
thread 'detail::tests::double_spend_tests::spend_bigger_output_in_the_same_block::case_1' panicked at 'explicit panic', chainstate/src/detail/tests/double_spend_tests.rs:292:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- detail::tests::double_spend_tests::spend_output_in_the_same_block::case_1 stdout ----
------------ TEST ARGUMENTS ------------
seed = Seed(533591205870860245)
-------------- TEST START --------------
thread 'detail::tests::double_spend_tests::spend_output_in_the_same_block::case_1' panicked at 'explicit panic', chainstate/src/detail/tests/double_spend_tests.rs:69:9


failures:
    detail::tests::double_spend_tests::spend_bigger_output_in_the_same_block::case_1
    detail::tests::double_spend_tests::spend_output_in_the_same_block::case_1

test result: FAILED. 3 passed; 2 failed; 0 ignored; 0 measured; 58 filtered out; finished in 0.00s

error: test failed, to rerun pass '-p chainstate --lib'

Also these test cases can be filtered by name

chainstate-storage/src/store.rs

chainstate/src/lib.rs

p2p/tests/ban.rs

p2p/tests/libp2p-gossipsub.rs

iljakuklic

Adding some comments. All minor suggestions or discussion points. I am happy with merging the PR as is from what I see.

chainstate/src/detail/orphan_blocks/pool.rs

chainstate/src/detail/mod.rs

chainstate/src/detail/tests/syncing_tests.rs

test-utils/src/random.rs

TheQuantumPhysicist · 2022-07-26T12:44:17Z

rstest with #[trace] attributte produce nice structured output that doesn't require us to write any additional macros.

cargo test --package chainstate --lib -- detail::tests::double_spend_tests

     Finished test [unoptimized + debuginfo] target(s) in 0.28s`
     Running unittests src/lib.rs (target/debug/deps/chainstate-c420b73216d9337b)

running 5 tests
test detail::tests::double_spend_tests::spend_output_in_the_same_block_invalid_order::case_1 ... ok
test detail::tests::double_spend_tests::spend_bigger_output_in_the_same_block::case_1 ... FAILED
test detail::tests::double_spend_tests::double_spend_tx_in_another_block::case_1 ... ok
test detail::tests::double_spend_tests::double_spend_tx_in_the_same_block::case_1 ... ok
test detail::tests::double_spend_tests::spend_output_in_the_same_block::case_1 ... FAILED

failures:

---- detail::tests::double_spend_tests::spend_bigger_output_in_the_same_block::case_1 stdout ----
------------ TEST ARGUMENTS ------------
seed = Seed(16783609377823135750)
-------------- TEST START --------------
thread 'detail::tests::double_spend_tests::spend_bigger_output_in_the_same_block::case_1' panicked at 'explicit panic', chainstate/src/detail/tests/double_spend_tests.rs:292:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- detail::tests::double_spend_tests::spend_output_in_the_same_block::case_1 stdout ----
------------ TEST ARGUMENTS ------------
seed = Seed(533591205870860245)
-------------- TEST START --------------
thread 'detail::tests::double_spend_tests::spend_output_in_the_same_block::case_1' panicked at 'explicit panic', chainstate/src/detail/tests/double_spend_tests.rs:69:9


failures:
    detail::tests::double_spend_tests::spend_bigger_output_in_the_same_block::case_1
    detail::tests::double_spend_tests::spend_output_in_the_same_block::case_1

test result: FAILED. 3 passed; 2 failed; 0 ignored; 0 measured; 58 filtered out; finished in 0.00s

error: test failed, to rerun pass '-p chainstate --lib'

Also these test cases can be filtered by name

That looks good. I think we can give that a shot.

TheQuantumPhysicist

I think this looks good now. A good step in the right direction.

iljakuklic

Cool stuff

Introduce proptest in chainstate

e4685cc

azarovh requested review from muursh, erubboli, iljakuklic, altonen and TheQuantumPhysicist as code owners July 20, 2022 08:47

iljakuklic approved these changes Jul 20, 2022

View reviewed changes

chainstate/src/detail/mod.rs Show resolved Hide resolved

chainstate/Cargo.toml Outdated Show resolved Hide resolved

stanislav-tkach approved these changes Jul 20, 2022

View reviewed changes

chainstate/src/detail/tests/double_spend_tests.rs Outdated Show resolved Hide resolved

chainstate/src/detail/tests/double_spend_tests.rs Outdated Show resolved Hide resolved

chainstate/src/detail/tests/double_spend_tests.rs Outdated Show resolved Hide resolved

muursh approved these changes Jul 20, 2022

View reviewed changes

TheQuantumPhysicist reviewed Jul 20, 2022

View reviewed changes

chainstate/src/detail/tests/events_tests.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist reviewed Jul 20, 2022

View reviewed changes

chainstate/src/detail/tests/processing_tests.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist reviewed Jul 20, 2022

View reviewed changes

chainstate/src/detail/tests/syncing_tests.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist reviewed Jul 20, 2022

View reviewed changes

chainstate/src/detail/tests/syncing_tests.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist approved these changes Jul 20, 2022

View reviewed changes

azarovh added 3 commits July 21, 2022 11:34

Fix review comments

b1aedca

Replace prop test wiht make_seedable_rng

70b45e7

Minor cleanup

5704598

azarovh commented Jul 22, 2022

View reviewed changes

chainstate/src/detail/tests/mod.rs Outdated Show resolved Hide resolved

chainstate/src/detail/tests/mod.rs Outdated Show resolved Hide resolved

Merge branch 'master' into proptest_in_chainstate

f41da5b

azarovh requested a review from TheQuantumPhysicist July 22, 2022 13:16

iljakuklic reviewed Jul 22, 2022

View reviewed changes

chainstate-storage/src/mock.rs Show resolved Hide resolved

chainstate/src/detail/tests/mod.rs Outdated Show resolved Hide resolved

chainstate/src/detail/tests/test_framework.rs Show resolved Hide resolved

TheQuantumPhysicist reviewed Jul 22, 2022

View reviewed changes

chainstate/src/detail/mod.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist reviewed Jul 22, 2022

View reviewed changes

chainstate/src/lib.rs Show resolved Hide resolved

azarovh added 2 commits July 24, 2022 11:05

Merge branch 'master' into proptest_in_chainstate

6a70ede

Introduce test-utils and make_seedable_rng macro

92741f8

azarovh commented Jul 25, 2022

View reviewed changes

test-utils/Cargo.toml Outdated Show resolved Hide resolved

iljakuklic closed this Jul 26, 2022

iljakuklic reopened this Jul 26, 2022

mintlayer deleted a comment from iljakuklic Jul 26, 2022

azarovh added 2 commits July 26, 2022 12:40

Move to rstest

2f2a673

Merge branch 'master' into proptest_in_chainstate

7d53e37

Fix build

1ea993c

azarovh requested review from iljakuklic, TheQuantumPhysicist and stanislav-tkach July 26, 2022 10:36

stanislav-tkach approved these changes Jul 26, 2022

View reviewed changes

chainstate-storage/src/store.rs Outdated Show resolved Hide resolved

chainstate/src/lib.rs Outdated Show resolved Hide resolved

p2p/tests/ban.rs Outdated Show resolved Hide resolved

p2p/tests/libp2p-gossipsub.rs Outdated Show resolved Hide resolved

iljakuklic approved these changes Jul 26, 2022

View reviewed changes

Fix review comments

5aa617e

TheQuantumPhysicist approved these changes Jul 26, 2022

View reviewed changes

Minor fixes

93bff2a

iljakuklic approved these changes Jul 26, 2022

View reviewed changes

TheQuantumPhysicist merged commit c3006ad into master Jul 26, 2022

TheQuantumPhysicist deleted the proptest_in_chainstate branch July 26, 2022 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce proptest in chainstate #294

Introduce proptest in chainstate #294

azarovh commented Jul 20, 2022

iljakuklic left a comment

stanislav-tkach left a comment

TheQuantumPhysicist left a comment

iljakuklic left a comment

iljakuklic commented Jul 25, 2022 •

edited

Loading

TheQuantumPhysicist commented Jul 25, 2022

azarovh commented Jul 25, 2022 •

edited

Loading

iljakuklic commented Jul 26, 2022

iljakuklic commented Jul 26, 2022

TheQuantumPhysicist commented Jul 26, 2022

iljakuklic commented Jul 26, 2022

azarovh commented Jul 26, 2022

iljakuklic left a comment

TheQuantumPhysicist commented Jul 26, 2022

TheQuantumPhysicist left a comment

iljakuklic left a comment

Introduce proptest in chainstate #294

Introduce proptest in chainstate #294

Conversation

azarovh commented Jul 20, 2022

iljakuklic left a comment

Choose a reason for hiding this comment

stanislav-tkach left a comment

Choose a reason for hiding this comment

TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

iljakuklic left a comment

Choose a reason for hiding this comment

iljakuklic commented Jul 25, 2022 • edited Loading

TheQuantumPhysicist commented Jul 25, 2022

azarovh commented Jul 25, 2022 • edited Loading

iljakuklic commented Jul 26, 2022

iljakuklic commented Jul 26, 2022

TheQuantumPhysicist commented Jul 26, 2022

iljakuklic commented Jul 26, 2022

azarovh commented Jul 26, 2022

iljakuklic left a comment

Choose a reason for hiding this comment

TheQuantumPhysicist commented Jul 26, 2022

TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

iljakuklic left a comment

Choose a reason for hiding this comment

iljakuklic commented Jul 25, 2022 •

edited

Loading

azarovh commented Jul 25, 2022 •

edited

Loading