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

chore: allow crater runs #8171

Merged

Conversation

bishopcheckmate
Copy link
Contributor

Motivation

Fixes #6924

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?

Specifications

This change adds versions to all dev-dependencies so that they end up being packed together with crates on crates.io thus allowing crater runs.

It also removes the circular dependency between tower-batch-control and zebra-consensus by copying the Ed25519 verifier code into batch-control tests. Also updates the verifier tests in consensus to their state from tower-batch-control.

There are still unadressed points in #6924:

Make sure the tests that are on by default:

  • run reasonably fast
  • don't use too much network
  • (check crater docs for any other requirements)

It's hard for me to define reasonably fast and too much networking. I can try to measure those on my PC if you want.
Regarding crater docs for any other requirements, I couldn't find any guidelines on that.

Testing

This should be tested using cargo publish, however it's not currently possible due to git dependencies added in #8136.

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.

@bishopcheckmate bishopcheckmate requested review from a team as code owners January 18, 2024 21:52
@bishopcheckmate bishopcheckmate requested review from arya2 and upbqdn and removed request for a team January 18, 2024 21:52
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 18, 2024
The timeouts copied from tower-batch-control seems too low
and there must have been a reasoning why were they introduced
arya2
arya2 previously approved these changes Jan 19, 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.

This looks great, thank you!

I left an optional suggestion for removing the dependency on zebra-chain and metrics.

Make sure the tests that are on by default:

run reasonably fast
don't use too much network
(check crater docs for any other requirements)

I didn't find any guidelines either, I'm not sure if there are any constraints in the code, or if it's manually observed and configured on a per-project basis here: https://github.com/rust-lang/crater/blob/master/config.toml

tower-batch-control/tests/ed25519.rs Show resolved Hide resolved
@arya2 arya2 added C-enhancement Category: This is an improvement A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-testing Category: These are tests and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Jan 19, 2024
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 21, 2024
arya2
arya2 previously approved these changes Jan 21, 2024
@upbqdn upbqdn removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 21, 2024
@oxarbitrage oxarbitrage added the do-not-merge Tells Mergify not to merge this PR label Jan 23, 2024
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 23, 2024
arya2
arya2 previously approved these changes Jan 23, 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.

This looks excellent, thank you!

@oxarbitrage oxarbitrage removed the do-not-merge Tells Mergify not to merge this PR label Jan 23, 2024
arya2
arya2 previously approved these changes Jan 24, 2024
@arya2
Copy link
Contributor

arya2 commented Jan 26, 2024

The CI failure seems unrelated to the changes, we should try to fix that or merge this manually.

arya2
arya2 previously approved these changes Jan 26, 2024
@oxarbitrage
Copy link
Contributor

@bishopcheckmate just FYI: your code is approved to be part of the zebra main branch but we are having CI issues with external pull requests. Right now, @gustavovalverde is working on fixing this issue. We will keep you posted, thanks for your contribution. I hope this problem do not discourage you to submit more code.

@bishopcheckmate
Copy link
Contributor Author

hey, no problem at all. I'll definitely keep contributing here and there, just been busy recently

zebra-scan/Cargo.toml Outdated Show resolved Hide resolved
arya2
arya2 previously approved these changes Feb 7, 2024
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.

Bumping all the versions to the ones in our current release.

tower-batch-control/Cargo.toml Outdated Show resolved Hide resolved
tower-batch-control/Cargo.toml Outdated Show resolved Hide resolved
tower-fallback/Cargo.toml Outdated Show resolved Hide resolved
zebra-chain/Cargo.toml Outdated Show resolved Hide resolved
zebra-consensus/Cargo.toml Outdated Show resolved Hide resolved
zebra-state/Cargo.toml Outdated Show resolved Hide resolved
zebrad/Cargo.toml Outdated Show resolved Hide resolved
zebrad/Cargo.toml Outdated Show resolved Hide resolved
zebrad/Cargo.toml Outdated Show resolved Hide resolved
zebrad/Cargo.toml Outdated Show resolved Hide resolved
zebrad/Cargo.toml Outdated Show resolved Hide resolved
@gustavovalverde gustavovalverde merged commit d1d128a into ZcashFoundation:main Feb 23, 2024
100 of 101 checks passed
idky137 pushed a commit to idky137/zebra that referenced this pull request Feb 28, 2024
* chore(dev-deps): always specify the version for dev-dependencies

* test(tower-batch-control): remove zebra-consensus dev dependency

Copies the Ed25519Verifier code directly to tower-batch-control tests

* test(zebra-consensus): update ed25519 verifier tests with ones from batch-control

* test(zebra-consensus): restore previous timeout values

The timeouts copied from tower-batch-control seems too low
and there must have been a reasoning why were they introduced

* chore: update dev-deps versions to beta.33

* chore(tower-batch-control): remove dev-dependency on metrics

* chore(tower-batch-control): remove zebra-chain dev-dependency

* Update zebra-scan/Cargo.toml

* bump all versions to match current release

* fix missed commas in version bumps

---------

Co-authored-by: Arya <aryasolhi@gmail.com>
Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
Co-authored-by: Pili Guerra <mpguerra@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Zebra crate versions to dev-dependencies, to enable crater tests
6 participants