-
Notifications
You must be signed in to change notification settings - Fork 710
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
Remove Mock Mempool #3687
base: master
Are you sure you want to change the base?
Remove Mock Mempool #3687
Conversation
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
} | ||
|
||
// Add should error if adding to the mempool errors | ||
func TestGossipMempoolAddError(t *testing.T) { |
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 test seems to just test the invariants of the mempool (too big/max size/etc) so I removed it since similar coverage exists elsewhere in this package
Signed-off-by: Joshua Kim <[email protected]>
appSenderFunc func(*gomock.Controller) common.AppSender | ||
expectedErr error | ||
} | ||
|
||
tests := []test{ | ||
{ | ||
name: "can't add transaction to mempool", |
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.
Similarly this test I also don't know if we want to keep... if we wanted to keep what this was intended to test we would probably want to add tests for the same mempool invariants described earlier
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Why this should be merged
Removes the usage of the mempool mock which makes tests brittle and difficult to reason about
How this works
Removes usage of mock and replaces it with the usage of the concrete mempool
How this was tested
UTs
Need to be documented in RELEASES.md?
No