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

Add zebra_state::init_test helper function for tests #2539

Merged

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Jul 28, 2021

Motivation

As part of the work for #1334, the construction of a state service instance will change a bit, because it will return a view to the current best chain tip block height. This will require changes to all tests that create a state service. To streamline these changes and to simplify the test code, this PR introduces an alternative constructor for tests to use.

Solution

The zebra_state::init_test function is available to other crates when the proptest-impl feature is enabled. The function creates a state service with an ephemeral Config and automatically wraps the service in a Buffer with one slot. These are the most common defaults for the tests, so it reduces a little the amount of repeated code.

Review

@teor2345 is reviewing the set of changes that are part of #1334.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

A follow-up PR will actually change the zebra_state::init method, but after this PR most tests will not need any extra changes.


This change is Reviewable

@jvff jvff requested a review from teor2345 July 28, 2021 01:07
@zfnd-bot zfnd-bot bot assigned jvff Jul 28, 2021
@teor2345
Copy link
Contributor

It seems like there are some changes missing from the commits in this PR?

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jvff)


zebra-state/src/service.rs, line 753 at r1 (raw file):
Can we document the ephemeral config and buffer size here?

The buffer size doesn't really matter, because the underlying service is always ready, and there won't be much contention in the tests. So it just needs to be documented.

/// Initialize a state service with an ephemeral [`Config`].
///
/// This can be used to create a state service for testing. See also [`init`].

zebra-state/src/service.rs, line 754 at r1 (raw file):

///
/// This can be used to create a state service for testing. See also [`init`].
#[cfg(any(test, feature = "proptest-impl"))]

Do some of the dependent crates need to activate zebra-chain's proptest-impl feature so they can see this function?


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

        .map(|block| Request::CommitFinalizedBlock(block.into()));

    let network = Network::Mainnet;

Adding buffers to these tests shouldn't change test behaviour.
But let's make sure CI passes before we merge.

jvff added 3 commits July 28, 2021 08:44
This function will be used as a replacement for `zebra_state::init`
inside tests. It's a simpler alternative because it can ignore any
details that aren't relevant for tests.
Update usages of `init` to use `init_test` instead, which simplifies
most cases.
Replace usages of `zebra_state::init` with the new helper function. This
simplifies the code a bit.
@jvff jvff force-pushed the add-zebra-state-init-test-helper-function branch from 6ee063f to ed1de3d Compare July 28, 2021 08:57
Copy link
Contributor Author

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Oops, yeah. Fixed now 😄

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @jvff and @teor2345)


zebra-state/src/service.rs, line 753 at r1 (raw file):

Previously, teor2345 (teor) wrote…

Can we document the ephemeral config and buffer size here?

The buffer size doesn't really matter, because the underlying service is always ready, and there won't be much contention in the tests. So it just needs to be documented.

/// Initialize a state service with an ephemeral [`Config`].
///
/// This can be used to create a state service for testing. See also [`init`].

Done.


zebra-state/src/service.rs, line 754 at r1 (raw file):

Previously, teor2345 (teor) wrote…

Do some of the dependent crates need to activate zebra-chain's proptest-impl feature so they can see this function?

Done.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jvff)

@jvff jvff merged commit 79d41b3 into ZcashFoundation:main Jul 28, 2021
@jvff jvff deleted the add-zebra-state-init-test-helper-function branch July 28, 2021 23:55
mpguerra added a commit that referenced this pull request Jul 29, 2021
mpguerra added a commit that referenced this pull request Jul 29, 2021
* Draft CHANGELOG for Zebra 1.0.0-alpha.14

* Add PR #2533 to CHANGELOG

* Apply suggestions from code review

Co-authored-by: teor <teor@riseup.net>

* Remove entry about updating the changelog

* move #2497

* add #2529

* Add a missing space

* Add #2458, #2525, #2486, #2542 and  #2539 to CHANGELOG

Co-authored-by: teor <teor@riseup.net>
Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants