test: Increase test coverage#2367
Conversation
WalkthroughThe changes remove a single, complex test function and its helper, replacing them with several new, focused test functions. These new tests independently verify getter methods, cache behavior, utility functions, error handling, and state management, including concurrency aspects, for the Changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
8c7a52e to
d25f75f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2367 +/- ##
==========================================
+ Coverage 69.79% 70.44% +0.65%
==========================================
Files 64 64
Lines 6267 6267
==========================================
+ Hits 4374 4415 +41
+ Misses 1501 1465 -36
+ Partials 392 387 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
block/manager_test.go (1)
180-201:⚠️ Potential issueFix inconsistent
IsDAIncludedsignature across tests
TestIsDAIncludedtreatsIsDAIncludedas returning a singlebool, whereas the newer tests expect(bool, error).
The mixed usage will fail to compile whichever signature you actually expose.Apply the patch below (or update the later tests in the opposite way) so all call-sites consistently expect the
(bool, error)form:@@ - // IsDAIncluded should return false for unseen hash - require.False(m.IsDAIncluded(ctx, height)) + // IsDAIncluded should return false for unseen hash + ok, err := m.IsDAIncluded(ctx, height) + require.NoError(err) + require.False(ok) @@ - m.headerCache.SetDAIncluded(header.Hash().String()) - require.False(m.IsDAIncluded(ctx, height)) + m.headerCache.SetDAIncluded(header.Hash().String()) + ok, err = m.IsDAIncluded(ctx, height) + require.NoError(err) + require.False(ok) @@ - m.dataCache.SetDAIncluded(data.DACommitment().String()) - require.True(m.IsDAIncluded(ctx, height)) + m.dataCache.SetDAIncluded(data.DACommitment().String()) + ok, err = m.IsDAIncluded(ctx, height) + require.NoError(err) + require.True(ok)Without this alignment the file will not type-check.
Also applies to: 695-705
🧹 Nitpick comments (2)
block/manager_test.go (2)
525-531: Redundant assertions onPendingHeadersrequire.Nil(result) require.Equal(m.pendingHeaders, result)
Equalis superfluous after assertingNil; both conditions reduce to the
same check. Dropping the second line will streamline the test.
553-556: Local variable shadows imported package name
cache := m.HeaderCache()hides thecachepackage imported at the top.
Renaming the variable (e.g.,hdrCache) avoids shadowing and improves
readability.- cache := m.HeaderCache() - require.NotNil(cache) - require.Equal(m.headerCache, cache) + hdrCache := m.HeaderCache() + require.NotNil(hdrCache) + require.Equal(m.headerCache, hdrCache)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
block/manager_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: test / Run E2E System Tests
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
block/manager_test.go (1)
611-625: Guard againstnilconfigwhen patching block-time
m.configis left at its zero value bygetManager.
Writing tom.config.DA.BlockTime.Durationwithout ensuring that
m.config(or the nested structs) are initialised will panic
if the real type uses pointers.Either initialise a minimal config in
getManager, or do it here:if m.config.DA == nil { // or the relevant zero check for your type m.config = defaultConfig() // helper returning a fully-formed config } m.config.DA.BlockTime.Duration = 10 * time.SecondThis keeps the test resilient to future refactors of
Manager.
a7b522a to
dd23696
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
block/manager_test.go (1)
775-839:⚠️ Potential issueUse
errors.Isfor error comparisons to handle wrapped errors.The error assertions use direct equality which will fail if the errors are wrapped in production code.
Apply this diff to make error assertions more robust:
- require.Equal(expectedErr, err) + require.True(errors.Is(err, expectedErr))This change should be applied to lines 784, 796, 819, and 837.
🧹 Nitpick comments (1)
block/manager_test.go (1)
1014-1018: Consider clarifying the test comment for better understanding.The test correctly validates that backoff is capped at 0 when block time is 0, but the comment could be more descriptive about the capping behavior.
- require.Equal(0*time.Millisecond, result) // Capped at 0 + require.Equal(0*time.Millisecond, result) // Capped at block time (0ms), not doubled
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
block/manager_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / Build Docker Image
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (7)
block/manager_test.go (7)
45-55: LGTM! Proper initialization of new Manager fields.The addition of the buffered
txNotifyChchannel (size 1) andsignaturePayloadProvideris correctly implemented. The buffered channel prevents blocking in the notification system, which is a good design choice.
596-638: Well-structured getter method tests!Good coverage of all getter methods with clear, focused test cases.
640-670: Comprehensive cache method testing!Excellent coverage of cache functionality including state transitions.
672-773: Excellent utility function test coverage!Thorough testing of edge cases, error conditions, and proper header validation logic.
841-915: Excellent thread safety testing!Comprehensive concurrency tests for state management with proper synchronization.
917-955: Well-designed notification system tests!Good coverage of both blocking and non-blocking scenarios. The timing assertion for non-blocking behavior is reasonable.
957-1006: Solid validation method tests!Good coverage of success and failure scenarios with proper configuration testing.
dd23696 to
cbd3ec4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
block/manager_test.go (3)
54-55: Channel capacity oftxNotifyChmight be too restrictiveA capacity of
1is fine whenNotifyNewTransactionsintentionally drops subsequent events while one is pending, but if the production consumer does not drain the channel promptly (e.g. under load, long GC pause, or bug) the notification path silently stops signalling new transactions. Consider documenting the intentional drop-behaviour or increasing the buffer (e.g.16) so that short bursts are not lost.- txNotifyCh: make(chan struct{}, 1), + // small buffer to survive short bursts without losing events + txNotifyCh: make(chan struct{}, 16),
617-623: Redundant assertion in “PendingHeaders” sub-test
require.Nil(result)already establishes that the value isnil; the subsequent equality check against the samenilpointer adds no extra safety and can be dropped for clarity.- require.Nil(result) - require.Equal(m.pendingHeaders, result) + require.Nil(result)
920-930: DraintxNotifyChafter burst to avoid leaking goroutinesAfter issuing 11 notifications only one value is read from the buffered channel, leaving 10 unread.
In a long-lived test suite this can block other goroutines that try to drain or close the channel.
Consider draining the channel (or replacing it with a local one) once the assertion is done.for len(m.txNotifyCh) > 0 { <-m.txNotifyCh }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
block/manager_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: lint / markdown-lint
- GitHub Check: test / Run Integration Tests
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Build Docker Image
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
block/manager_test.go (1)
1015-1018: Zero block-time cap semantics need clarificationThe test expects that when
m.config.DA.BlockTime.Duration == 0, the back-off is forcibly clamped to zero.
Most back-off implementations treat “0” as “no cap”, returning the doubled value instead.
Please confirm that the productionexponentialBackoffintentionally clamps to0, otherwise this assertion will fail and mask real regressions.
| t.Run("NotifyNewTransactions_NonBlocking", func(t *testing.T) { | ||
| require := require.New(t) | ||
| m, _ := getManager(t, mocks.NewDA(t), -1, -1) | ||
|
|
||
| // Fill the channel to test non-blocking behavior | ||
| m.txNotifyCh <- struct{}{} | ||
|
|
||
| // This should not block even though channel is full | ||
| start := time.Now() | ||
| m.NotifyNewTransactions() | ||
| duration := time.Since(start) | ||
|
|
||
| // Should complete quickly (non-blocking) | ||
| require.Less(duration, 10*time.Millisecond) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tight 10 ms wall-clock assertion risks flakiness in CI
require.Less(duration, 10*time.Millisecond) can intermittently fail on loaded CI runners or slow virtualised environments.
Either relax the bound (e.g. 100 ms) or use assert.Eventually / assert.Never with a context to express non-blocking semantics without relying on wall-clock precision.
- require.Less(duration, 10*time.Millisecond)
+ require.Less(duration, 100*time.Millisecond) // avoid test flakiness on slow machines📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("NotifyNewTransactions_NonBlocking", func(t *testing.T) { | |
| require := require.New(t) | |
| m, _ := getManager(t, mocks.NewDA(t), -1, -1) | |
| // Fill the channel to test non-blocking behavior | |
| m.txNotifyCh <- struct{}{} | |
| // This should not block even though channel is full | |
| start := time.Now() | |
| m.NotifyNewTransactions() | |
| duration := time.Since(start) | |
| // Should complete quickly (non-blocking) | |
| require.Less(duration, 10*time.Millisecond) | |
| }) | |
| t.Run("NotifyNewTransactions_NonBlocking", func(t *testing.T) { | |
| require := require.New(t) | |
| m, _ := getManager(t, mocks.NewDA(t), -1, -1) | |
| // Fill the channel to test non-blocking behavior | |
| m.txNotifyCh <- struct{}{} | |
| // This should not block even though channel is full | |
| start := time.Now() | |
| m.NotifyNewTransactions() | |
| duration := time.Since(start) | |
| // Should complete quickly (non-blocking) | |
| require.Less(duration, 100*time.Millisecond) // avoid test flakiness on slow machines | |
| }) |
🤖 Prompt for AI Agents
In block/manager_test.go around lines 940 to 954, the test uses a strict 10
millisecond duration check to assert non-blocking behavior, which can cause
flaky failures in CI environments. To fix this, relax the timing bound to a
higher value like 100 milliseconds or replace the direct duration check with
assert.Eventually or assert.Never combined with a context to more reliably
verify non-blocking behavior without depending on precise timing.
2 participants
Overview
Summary by CodeRabbit