-
Notifications
You must be signed in to change notification settings - Fork 54
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
test: chainReader
shares and operatorSets functions
#437
test: chainReader
shares and operatorSets functions
#437
Conversation
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.
LGTM, just leave you some comments:
|
||
const allocationDelay = 1 | ||
const allocationMagnitude = 100 | ||
const allocationConfigurationDelay = 1200 |
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.
These constants should be outside the test or else they can be variables.
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.
Done 9d32443
true, | ||
) | ||
require.NoError(t, err) | ||
require.Equal(t, uint64(1), receipt.Status) |
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 literal can be replaced for gethtypes.ReceiptStatusSuccessful
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.
Done 4fbf686
opSets, err := chainReader.GetOperatorSetsForOperator(context.Background(), operatorAddr) | ||
require.NoError(t, err) | ||
require.NotEmpty(t, opSets) | ||
require.Len(t, opSets, 1) |
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 opSets has len 1 then it is not empty, so one require is redundant in my opinion
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.
Done 1e9bf2d
operators, err := chainReader.GetOperatorsForOperatorSet(context.Background(), operatorSet) | ||
require.NoError(t, err) | ||
require.NotEmpty(t, operators) | ||
require.Len(t, operators, 1) |
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.
Ditto
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.
Done 1e9bf2d
What Changed?
This PR aims to increase the test coverage of
reader.go
.Tested functions:
GetOperatorsForOperatorSet
GetNumOperatorsForOperatorSet
GetOperatorSetsForOperator
GetNumOperatorSetsForOperator
GetStrategiesForOperatorSet
GetSlashableShares
GetSlashableSharesForOperatorSets
GetSlashableSharesForOperatorSetsBefore
GetRegisteredSets
Increased the test timeout from 4 minutes to 5 minutes.
Reviewer Checklist