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

Combine proptests and transcripts to perform top-level integration tests #1021

Closed
2 tasks
Tracked by #745
teor2345 opened this issue Sep 7, 2020 · 2 comments
Closed
2 tasks
Tracked by #745
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement S-blocked Status: Blocked on other tasks

Comments

@teor2345
Copy link
Contributor

teor2345 commented Sep 7, 2020

Is your feature request related to a problem? Please describe.

In #918, we wanted to move the block time check tests to zebra-consensus, but they depend on zebra-chain::generate.

  • detailed explanation:
    • is_time_valid_at is also used in test code, specifically chain/src/block/tests/vectors.rs.
    • These tests should be moved into the consensus crate.
    • Some of the tests can't be moved, because they rely on the generate code, which isn't exposed to the rest of the crate API

Describe the solution you'd like

  • The generate code should be expressed as proptest strategies (Rewrite block::tests::generate as proptest strategies. #919) to fix this.
  • Then consensus tests can be written using the proptest strategies.
  • In fact, rather than writing unit tests, we could combine proptests with the transcript functionality to perform top-level integration tests.
  • Doing this correctly requires some amount of work but will save a lot of work compared to writing individual unit tests.

Post-implementation cleanup

  • Inline zebra_chain::block::Header::is_time_valid_at into zebra_consensus::block::check::is_time_valid_at
  • Make zebra_chain::block::header::BlockTimeError::InvalidBlockTime the source of zebra_consensus::error::BlockError::Time

My intuition is that we might not want to inline this, and instead take the first error message defined here, "invalid time in block header" and make that the error message on the variant in BlockError. Then we make BlockTimeError #[source] for BlockError::Time and have it only print "must be less than 2 hours in the future {0:?}" as its error message. This way we can let the reporter split the msg into multiple lines if it wants to which will make our error reports nicer imo.

Originally posted by @yaahc in #1146 (comment)

Describe alternatives you've considered

Keep the current implementation, which generates blocks without using proptests.

Additional context

This issue is blocked on #919

@teor2345 teor2345 added E-med A-rust Area: Updates to Rust code E-help-wanted Call for participation: Help is requested to fix this issue. S-blocked Status: Blocked on other tasks C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup E-needs-test Call for participation: Writing correctness tests labels Sep 7, 2020
@teor2345 teor2345 added the S-needs-triage Status: A bug report needs triage label Sep 8, 2020
@teor2345 teor2345 removed E-help-wanted Call for participation: Help is requested to fix this issue. E-needs-test Call for participation: Writing correctness tests labels Sep 17, 2020
@teor2345
Copy link
Contributor Author

Removing the volunteer tags from this issue, until it is unblocked.

@teor2345 teor2345 added this to the Block Validation milestone Sep 29, 2020
@mpguerra mpguerra removed this from the Block Validation milestone Jan 5, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Jan 15, 2021
@mpguerra mpguerra removed the E-med label Mar 23, 2021
@teor2345
Copy link
Contributor Author

This is a good idea, but the tests are mostly ok. It might also be obsolete.

@mpguerra mpguerra closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
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 S-blocked Status: Blocked on other tasks
Projects
None yet
Development

No branches or pull requests

2 participants