-
Notifications
You must be signed in to change notification settings - Fork 30
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
[CX_HARDENING] - Develop A Better Framework for Input Permutation #3207
Conversation
@@ -179,6 +179,15 @@ pub struct Expectations<TYPES: NodeType, S> { | |||
pub task_state_asserts: Vec<Box<dyn Predicate<S>>>, | |||
} | |||
|
|||
impl<TYPES: NodeType, S> Expectations<TYPES, S> { | |||
pub fn from_outputs(output_asserts: Vec<Box<dyn Predicate<Arc<HotShotEvent<TYPES>>>>>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ss-es I added this because we only assert in two tests in the entire codebase, so it was easier than writing the same line over and over, and I don't think it hurts readability, though I could be swayed on a different name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Honestly, you could probably even just call it outputs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me if CI passes
Closes #3161
This PR:
Inputs are mostly written for the happy path. The existing tests are written with a single stream of inputs, and this has bitten us in the past. However, as we see in the proposal_ordering tests, the permutation with index order is not sufficient and requires the implementor to arduously call a significant number of cases with manually prescribed indices. We can do better. We need a scalable and reliable way to permute inputs (and outputs) in a way that ensures that all possible orderings can be handled by the test.
This PR does just that, it adds a randomization layer over the inputs which can be specified by the new
random
andserial
macros. These macros make it easier to declare a vec with the context that you're intending for it to be used in without any additional thought.The new test script here will eventually completely replace the old one in
script.rs
This PR does not:
This PR does not rewrite every test, that would make the PR large and any changes would be painful. I have updated one test and will update the rest in a follow on PR
Key places to review: