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

fix: Speed up the block round trip proptest #817

Merged
merged 1 commit into from Aug 5, 2020

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 4, 2020

Reduce the number of cases run by the block round trip proptest, to
speed up the Zebra tests.

Closes #803.

Reduce the number of cases run by the block round trip proptest, to
speed up the Zebra tests.
@teor2345 teor2345 added Poll::Ready A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup labels Aug 4, 2020
@teor2345 teor2345 requested review from yaahc and a team August 4, 2020 04:38
@teor2345 teor2345 added this to In progress in 🦓 via automation Aug 4, 2020
@teor2345 teor2345 self-assigned this Aug 4, 2020
@teor2345
Copy link
Contributor Author

teor2345 commented Aug 4, 2020

After this change, using this script:

for d in zebra*; do
    pushd "$d"
    cargo --quiet test --no-run
    time cargo --quiet test
    popd
done

The crates with test times over 1 second are:

zebra-chain
real 0m8.927s
user 0m39.625s

zebra-consensus
real 0m1.716s
user 0m2.761s

zebrad
real 0m3.837s
user 0m1.772s

@teor2345
Copy link
Contributor Author

teor2345 commented Aug 4, 2020

@yaahc is 15-20 seconds fast enough for all the Zebra tests, or would you like me to tweak them some more?

Copy link
Contributor

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

15-20 seconds is plenty good I expect, lol.

🦓 automation moved this from In progress to Reviewer approved Aug 4, 2020
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

I'm ok with configuring the number of proptests run, I wonder if we can use the env vars it looks at to make it 'faster' where needed (local before push) and 'slower' where capacity allows (CI, where we can require even more test cases to pass if we want).

https://docs.rs/proptest/0.10.0/proptest/prelude/struct.ProptestConfig.html#structfield.cases

image

zebra-chain/src/block/tests.rs Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

teor2345 commented Aug 4, 2020

I'm ok with configuring the number of proptests run, I wonder if we can use the env vars it looks at to make it 'faster' where needed (local before push) and 'slower' where capacity allows (CI, where we can require even more test cases to pass if we want).

I don't want to globally override the number of cases by default, because only the block rests are slow right now.

But I could use the cases env var where it exists, and then default to 16 block cases. That way, we can increase the number of cases in CI, or decrease them even further right before push.

(Most of the time when I'm developing, each change takes me longer than 20 seconds to write.)

Copy link
Contributor

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

lgtm

@hdevalence hdevalence merged commit ac7a4ae into ZcashFoundation:main Aug 5, 2020
🦓 automation moved this from Reviewer approved to Done Aug 5, 2020
@teor2345
Copy link
Contributor Author

teor2345 commented Aug 5, 2020

But I could use the cases env var where it exists, and then default to 16 block cases. That way, we can increase the number of cases in CI, or decrease them even further right before push.

See #828 for this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup
Projects
No open projects
🦓
  
Done
Development

Successfully merging this pull request may close these issues.

slow test cleanup
4 participants