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

Adding ScenarioBuilder #4812

Merged
merged 10 commits into from
Sep 11, 2021
Merged

Adding ScenarioBuilder #4812

merged 10 commits into from
Sep 11, 2021

Conversation

posvyatokum
Copy link
Member

Adding an easier way to create basic scenarios.

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

It's great that we have documentation for how this thing works!

Comment on lines +22 to +23
height: 1,
nonce: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we start from 0 instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I forgot why, but block at height 0 could not be produced

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

LGTM overall!

I like the docs, but I think they might not survive the merciless grind of time in the current form :-) I suggest pushing most of them into the API comments and doc tests

Comment on lines +8 to +11
pub mod scenario_builder;

pub use crate::run_test::{BlockConfig, NetworkConfig, Scenario, TransactionConfig};
pub use crate::scenario_builder::ScenarioBuilder;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a minor code smell -- now the caller can use ScenarioBuilder as either runtime_tested::ScenarioBuilder or runtime_tester::scenario_builer::ScenarioBuilder, and it's not immediately clear from the code which variant is preferred.

I usually prefer to make API with only one possilbe import path. For smaller crates, it's usually best for the caller if all the API is available at root, and all the internals are private.

But that's a minor point, the code looks ok as is!

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.

3 participants