-
Notifications
You must be signed in to change notification settings - Fork 203
chore(node/actor): Test: Empty Attributes while building unsealed payload #3172
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
base: main
Are you sure you want to change the base?
chore(node/actor): Test: Empty Attributes while building unsealed payload #3172
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. |
op-will
left a comment
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! You could merge as-is, but how do you feel about updating the test to handle all possible prepare_payload_attributes errors?
| let err = result.unwrap_err(); | ||
| assert!(matches!( | ||
| err, | ||
| SequencerActorError::AttributesBuilder(PipelineErrorKind::Critical(_)) |
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.
While we can rely on the default error returned from the default TestAttributesBuilder, that may change, causing this test to break. It may be better to:
- Define different error types for
TestAttributesBuilderError(Temporary, Critical, Reset) -- or even better, remove that error type and just usePipelineErrorKind - Update
TestAttributesBuilder.prepare_payload_attributesto return thePipelineErrorKindof the configured error - Create and use a
TestAttributesBuilder, add the exact error you want to receive to theattributesfield - Assert that the error exactly matches the returned error
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.
This also lets you parameterize this test to verify all error types (example) are handled how you expect them to be handled in a single test.
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
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.
How do you feel about creating an actor_test.rs file and moving these tests/utils to it?
There are many branches in this logic, so there will eventually be many lines of unit tests, so we can de-clutter the live logic by moving to a separate file.
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.
The way I'd go about it would be to have a test folder which is feature gated with #[cfg(test)] and put all the tests there
| use crate::actors::{MockBlockBuildingClient, MockOriginSelector}; | ||
|
|
||
| #[tokio::test] | ||
| async fn test_build_unsealed_payload_no_attributes() { |
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.
We're really testing the handling of prepare_payload_attributes errors.
| async fn test_build_unsealed_payload_no_attributes() { | |
| async fn test_build_unsealed_payload_prepare_payload_attributes_error() { |
| use tokio_util::sync::CancellationToken; | ||
|
|
||
| // Returns a test SequencerActorBuilder with mocks that can be used or overridden. | ||
| pub(crate) fn test_builder() -> SequencerActorBuilder< |
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.
I think we would want to move this to a test_util.rs file since it's used in multiple files, but it is being removed soon: #3152
Although even without the builder, we'll still probably have some utils around creating the test SequencerActor.
Adds one unit test which checks No attributes available but building unsealed payload triggered, leading critical error.
Moved test_builder helper to actor for reusability.
Part of #3070