test: mempool: Add unit and integration tests #8017
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8017 +/- ##
==========================================
+ Coverage 39.13% 39.22% +0.08%
==========================================
Files 662 663 +1
Lines 72160 72171 +11
==========================================
+ Hits 28242 28310 +68
+ Misses 39008 38953 -55
+ Partials 4910 4908 -2
Continue to review full report at Codecov.
|
|
@TheMenko tests failing now |
|
@ZenGround0 Seems we have a flake, I may have just died a little bit inside. 🥲 |
|
I have the ability to rerun failures but since the test is flaky that won't do much good because a failure always means you need another push, it's rerunning successes that you're interested in. In general I would run it under right conditions and enough times locally to get a failure reproduction, make the change that removes the flake and try to reproduce again locally if you're unsure. Usually understanding the root cause of the flake and convincing myself that it's gone is enough for me. In the rare case local reproduction is just not happening then after making the change I was convinced removed the flake I would make a bunch of noop changes and push enough times to convince myself with high probability the flake was gone. |
|
@ZenGround0 Thanks for the detailed explanation! We will find a way to reproduce and fix it locally and we'll get back to you after with an explanation! 🙂 |
…to bloxico/mempool_tests
The flake was caused by improper waiting for certain chain operations to finish: - We didn't wait for messages to be registered as pushed - We improperly waited for a fixed time (10 seconds) for messages to be mined, which in the best case would wait longer than necessary and in the worst case would cause the test to break. What I did: - fixed by waiting in a loop for "just enough time". This fixed the flake and made the test run faster, on average, because we don't have unnecessary waiting. - I added a "circuit-breaker" where the wait loop will timeout after 10 seconds.
Using the same pattern described in my previous commit. I also added the CircuitBreaker to the itests kit as it may be useful for other integration tests when debugging flakyness caused by timeouts.
|
@ZenGround0 I think this PR is ready to be merged now. |
| You can use it if t.Deadline() is not "granular" enough, and you want to know which specific piece of code timed out, | ||
| or you need to set different deadlines in the same test. | ||
| */ | ||
| func CircuitBreaker(t *testing.T, label string, throttle, timeout time.Duration, cb func() bool) { |
Related Issues
There were 0 existing messagepool integration tests. Since the pool was only being tested as a side-effect of other features and integration tests, adding tests targeting only the messagepool will allow easier debugging, code safety, and improve coverage.
Proposed Changes
Add more unit and integration tests covering untested behaviors:
check_test.go, andAddmessage tests inmessagepool_test.go)batch push,remove, etc.Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, testarea: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps