-
Notifications
You must be signed in to change notification settings - Fork 222
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
Add Unit Test for Test Aggregate Signatures #936
Conversation
warp/aggregator/aggregator_test.go
Outdated
require.ErrorIs(t, err, context.Canceled) | ||
return nil, err | ||
default: | ||
// We should not be able to get this signature since context cancelled in another go routine |
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.
if this is the case then is it necessary to add this expected call to the mock?
if it's necessary, could we factor out the common code here (eg calling client.EXPECT()
in a loop)
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 see, I can remove the default case then. I just thought it would be more clear that this case would NOT happen.
Why this should be merged
This unit test simulates the scenario where the context is canceled midway through signature fetching.
All signature fetching go routines prior to cancellation should be able to get a signature.
All signature fetching go routines after cancellation should return with context cancelled error and no signature.
How this works
The result should have an aggregate signature from only one validator.
How this was tested
Only adds a UT. No actual change to any prod code.
How is this documented
UT is commented.