-
Couldn't load subscription status.
- Fork 159
test: Fix mempool flakes #1360
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: Fix mempool flakes #1360
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.
I'm also not positive this will fix the flakes, but the locking makes sense to me.
bfd82ee to
34355e9
Compare
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.
Added comments for what these tests previously actually did.
We should decide if that behavior is worth testing (and if it is we should document it correctly).
4bdb916 to
2a6d166
Compare
Signed-off-by: Stephen Buttolph <stephen@avalabs.org>
| // Tests that conflicting txs are handled, based on whether the original tx is | ||
| // valid or not. | ||
| func TestAtomicTxPushGossipInboundConflicting(t *testing.T) { | ||
| require := require.New(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.
You don't need this. The only thing it achieves is implicitly passing testing.T to the concrete require assertion call, but it adds unnecessary noise.
Why this should be merged
Closes #1356 as pre-req for monorepo. These tests were hazy in scope anyway and didn't accurately test what it intended to.
How this works
It removes one of the tests, and combines two of the tests utilizing the
AppGossipfunction instead.How this was tested
UT
Need to be documented?
No
Need to update RELEASES.md?
No