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

change(tests): Remove Matches on Network From Tests #8295

Merged
merged 22 commits into from
Mar 5, 2024

Conversation

idky137
Copy link
Contributor

@idky137 idky137 commented Feb 20, 2024

Motivation

We would like to implement regtest mode in zebrad to allow development to move away from relying on zcashd for testing. (https://forum.zcashcommunity.com/t/zingo-onward/43945/44)

As Per #7967 we have decided to implement this in multiple stages, to ease the process of merging code.

This PR completes point 1 in #7968.

Closes #8325.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Solution

We have added a new set of methods for Network in Zebra-chain to remove the reliance on matching on the Network type and to move code fetching test vectors into a single file.

Testing

This PR effects tests in Zebra-Chain, Zebra-Consensus, Zebra-RPC, Zebra-GRPC, Zebra-Scan, Zebra-State and Zebrad.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Feb 20, 2024
@mpguerra
Copy link
Contributor

Thank you for the PR! We will review and discuss as a team to decide how to proceed with this one.

@AloeareV
Copy link
Contributor

We're following the steps here #7968 as a starting point

@zancas
Copy link
Contributor

zancas commented Feb 23, 2024

@AloeareV this #7967 (comment) is more detailed than #7968.

@idky137
Copy link
Contributor Author

idky137 commented Feb 23, 2024

Currently implements the first point in #7968. However new vector fetching functionality has been implemented as methods on Network, held in zebra-chain/src/test_utils/.

@idky137 idky137 changed the title Add regtest mode Remove Matches on Network From Tests Feb 23, 2024
@idky137
Copy link
Contributor Author

idky137 commented Feb 23, 2024

changed scope of PR to ease merging.

@github-actions github-actions bot added the extra-reviews This PR needs at least 2 reviews to merge label Feb 23, 2024
@arya2 arya2 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Feb 23, 2024
@AloeareV
Copy link
Contributor

fixes #8325, thanks for issuing this!

Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Thank you for these changes!!

The first 13 commits are looking good and ready to merge once CI passes.

The only change I think is needed is removing the serialization error variant or hiding it behind #[cfg(test)].

If there's interest in implementing #8326 or #7968, please do these in follow-up PRs so the changes are easy to test and review.

We want the Zebra team to implement the issues following #7968 in #7845 for adding parameters, because those changes may have far-reaching implications across the codebase, and we expect context and familiarity with the Zebra codebase will make it easier to implement those changes correctly.

Note: We just started a hack sprint, so reviews on those changes may be delayed until our next sprint.

zebra-chain/src/test_utils/vectors.rs Outdated Show resolved Hide resolved
zebra-chain/src/serialization/error.rs Outdated Show resolved Hide resolved
zebra-chain/src/test_utils/vectors.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/tests/vectors.rs Outdated Show resolved Hide resolved
arya2
arya2 previously approved these changes Mar 1, 2024
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Thank you!

@arya2 arya2 marked this pull request as ready for review March 1, 2024 18:55
@arya2 arya2 requested review from a team as code owners March 1, 2024 18:55
@arya2 arya2 requested review from upbqdn and removed request for a team March 1, 2024 18:55
@arya2 arya2 changed the title Remove Matches on Network From Tests change(tests): Remove Matches on Network From Tests Mar 1, 2024
zebra-chain/src/test.rs Outdated Show resolved Hide resolved
@arya2 arya2 merged commit 9b91d4b into ZcashFoundation:main Mar 5, 2024
99 checks passed
@arya2 arya2 added this to the Regtest Network support milestone Mar 20, 2024
@arya2 arya2 mentioned this pull request Apr 15, 2024
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG extra-reviews This PR needs at least 2 reviews to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor tests to use is_mainnet() to look up test vectors using new functions in zebra-test
5 participants