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

Make nullifier tests faster and consistent with UTXO tests #2513

Merged
merged 7 commits into from
Jul 23, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 21, 2021

Motivation

The nullifier tests are a bit slow on some developers' machines.

This PR also makes minor changes to the nullifier test code to match the UTXO tests in PR #2511.

This PR does not close any tickets.

Solution

  • reduce the number of proptest cases for slow state tests
  • rename tests for consistency and clarity
  • make some non-finalized state methods test-only

Review

@oxarbitrage can review this PR. It's not urgent at all.

Reviewer Checklist

  • Refactors make sense
  • Tests are faster

This change is Reviewable

@teor2345 teor2345 added NU-1 Sapling Network Upgrade: Sapling specific tasks NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup NU-5 Network Upgrade: NU5 specific tasks labels Jul 21, 2021
@teor2345 teor2345 self-assigned this Jul 21, 2021
The state tests should be about 4x faster after these changes.

They reduce total state test "user CPU" time to 20-30 seconds on my
machine. Previously it was around 2 minutes.
@teor2345 teor2345 changed the title Make nullifier tests consistent with UTXO tests Make nullifier tests faster and consistent with UTXO tests Jul 22, 2021
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @teor2345)


zebra-state/src/service/non_finalized_state/tests/prop.rs, line 17 at r1 (raw file):

};

const DEFAULT_PARTIAL_CHAIN_PROPTEST_CASES: u32 = 16;

I think it will be good to change this one as well: https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/non_finalized_state/tests/prop.rs#L17 but we can do it in a separated PR.

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @teor2345)


zebra-state/src/service/non_finalized_state/tests/prop.rs, line 17 at r1 (raw file):

Previously, oxarbitrage (Alfredo Garcia) wrote…

I think it will be good to change this one as well: https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/non_finalized_state/tests/prop.rs#L17 but we can do it in a separated PR.

Sorry, i posted the link for the one changed. I wanted to say to make an additional change here: https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/finalized_state/tests/prop.rs#L15

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @teor2345)


zebra-state/src/service/non_finalized_state/tests/prop.rs, line 17 at r1 (raw file):

Previously, oxarbitrage (Alfredo Garcia) wrote…

Sorry, i posted the link for the one changed. I wanted to say to make an additional change here: https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/finalized_state/tests/prop.rs#L15

I opened a PR for this at #2521

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @teor2345)

@oxarbitrage oxarbitrage enabled auto-merge (squash) July 23, 2021 14:01
@oxarbitrage oxarbitrage merged commit 2363889 into ZcashFoundation:main Jul 23, 2021
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 C-enhancement Category: This is an improvement NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) NU-1 Sapling Network Upgrade: Sapling specific tasks NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants