fix: Dummy test#2300
Conversation
WalkthroughThe changes export the Changes
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 Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ 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).
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| t.Run("non-existent rollup ID", func(t *testing.T) { | ||
| // Try to get a batch for a non-existent rollup ID | ||
| nonExistentRollupID := []byte("non-existent-rollup") | ||
| _, err := seq.GetNextBatch(ctx, GetNextBatchRequest{ | ||
| req, err := seq.GetNextBatch(ctx, GetNextBatchRequest{ | ||
| RollupId: nonExistentRollupID, | ||
| }) | ||
|
|
||
| // Should return an error | ||
| if err == nil { | ||
| t.Fatal("GetNextBatch should return an error for non-existent rollup ID") | ||
| if err != nil { | ||
| t.Fatalf("no error expected: %s", err) | ||
| } | ||
| if req == nil || req.Batch == nil { | ||
| t.Fatal("unexpected nil response") | ||
| } | ||
| if err.Error() == "" || !contains(err.Error(), "no batch found for rollup ID") { | ||
| t.Fatalf("Error message should indicate the rollup ID was not found, got: %v", err) | ||
| if len(req.Batch.Transactions) != 0 { | ||
| t.Error("batch should be empty") | ||
| } |
There was a problem hiding this comment.
The test case name "non-existent rollup ID" and its comments suggest it's testing error handling, but the updated assertions now expect success with an empty batch. This creates a disconnect between the test's name/description and its actual behavior. Consider either:
- Updating the test name and comments to reflect the new expected behavior (e.g., "returns empty batch for unknown rollup ID"), or
- Reverting the assertions to match the original error-checking intent
This will ensure the test documentation accurately reflects what's being tested, making the code more maintainable and easier to understand for future developers.
| t.Run("non-existent rollup ID", func(t *testing.T) { | |
| // Try to get a batch for a non-existent rollup ID | |
| nonExistentRollupID := []byte("non-existent-rollup") | |
| _, err := seq.GetNextBatch(ctx, GetNextBatchRequest{ | |
| req, err := seq.GetNextBatch(ctx, GetNextBatchRequest{ | |
| RollupId: nonExistentRollupID, | |
| }) | |
| // Should return an error | |
| if err == nil { | |
| t.Fatal("GetNextBatch should return an error for non-existent rollup ID") | |
| if err != nil { | |
| t.Fatalf("no error expected: %s", err) | |
| } | |
| if req == nil || req.Batch == nil { | |
| t.Fatal("unexpected nil response") | |
| } | |
| if err.Error() == "" || !contains(err.Error(), "no batch found for rollup ID") { | |
| t.Fatalf("Error message should indicate the rollup ID was not found, got: %v", err) | |
| if len(req.Batch.Transactions) != 0 { | |
| t.Error("batch should be empty") | |
| } | |
| t.Run("returns empty batch for unknown rollup ID", func(t *testing.T) { | |
| // When requesting a batch for an unknown rollup ID, the sequencer should | |
| // return an empty batch rather than an error | |
| unknownRollupID := []byte("non-existent-rollup") | |
| req, err := seq.GetNextBatch(ctx, GetNextBatchRequest{ | |
| RollupId: unknownRollupID, | |
| }) | |
| if err != nil { | |
| t.Fatalf("no error expected: %s", err) | |
| } | |
| if req == nil || req.Batch == nil { | |
| t.Fatal("unexpected nil response") | |
| } | |
| if len(req.Batch.Transactions) != 0 { | |
| t.Error("batch should be empty") | |
| } | |
| }) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2300 +/- ##
==========================================
+ Coverage 55.75% 55.79% +0.03%
==========================================
Files 53 53
Lines 5008 5008
==========================================
+ Hits 2792 2794 +2
+ Misses 1931 1929 -2
Partials 285 285 ☔ 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: 0
🔭 Outside diff range comments (1)
core/sequencer/dummy.go (1)
30-39:⚠️ Potential issueCheck for uninitialized batchSubmissionChan
The method accesses
s.batchSubmissionChanwithout checking if it's initialized. This could lead to a nil pointer panic ifSetBatchSubmissionChanwasn't called.Add a nil check to avoid potential panics:
func (s *DummySequencer) SubmitRollupBatchTxs(ctx context.Context, req SubmitRollupBatchTxsRequest) (*SubmitRollupBatchTxsResponse, error) { s.mu.Lock() defer s.mu.Unlock() s.batches[string(req.RollupId)] = req.Batch - if req.Batch != nil && len(req.Batch.Transactions) > 0 { + if req.Batch != nil && len(req.Batch.Transactions) > 0 && s.batchSubmissionChan != nil { s.batchSubmissionChan <- *req.Batch } return &SubmitRollupBatchTxsResponse{}, nil }
🧹 Nitpick comments (1)
core/sequencer/dummy.go (1)
64-66: Consider initializing the channel in the constructorThe
batchSubmissionChanis only initialized via this setter method, which could lead to bugs if someone forgets to call it before using the sequencer.Consider initializing the channel in the constructor with a default buffer size:
func NewDummySequencer() *DummySequencer { return &DummySequencer{ batches: make(map[string]*Batch), + batchSubmissionChan: make(chan Batch, 10), // Default buffer size } }Then update the setter to allow overriding the default:
func (s *DummySequencer) SetBatchSubmissionChan(batchSubmissionChan chan Batch) { + // Close existing channel if it exists and isn't the same + if s.batchSubmissionChan != nil && s.batchSubmissionChan != batchSubmissionChan { + close(s.batchSubmissionChan) + } s.batchSubmissionChan = batchSubmissionChan }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
core/sequencer/dummy.go(3 hunks)core/sequencer/dummy_test.go(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/sequencer/dummy.go (1)
sequencers/single/sequencer.go (1)
Sequencer(30-44)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (9)
core/sequencer/dummy.go (3)
13-13: Good addition of interface compliance checkAdding the compile-time type assertion ensures that
DummySequencerproperly implements theSequencerinterface.
15-16: Export of type improves testabilityExporting the
DummySequencertype makes it accessible for testing in other packages. The additional documentation clarifies its purpose.
23-24: Updated constructor return type properly matches new exported typeThe constructor now correctly returns the concrete
*DummySequencertype instead of the interface, which makes initialization in tests more explicit.core/sequencer/dummy_test.go (6)
11-13: Good practice: Explicit channel initialization prevents deadlocksSetting the batch submission channel explicitly before using it prevents potential deadlocks when sending to an uninitialized or unbuffered channel.
53-57: Improved test setup with explicit channel initializationSetting the batch submission channel and context before the test cases improves test clarity and prevents potential deadlocks.
60-73: Updated expectations for non-existent rollup IDsThe test now correctly verifies that the sequencer returns an empty batch (with no transactions) instead of an error when querying a non-existent rollup ID. This matches the implementation behavior in the
GetNextBatchmethod.
147-158: Fix for potential deadlock in concurrency testThe batch submission channel is now properly initialized with a sufficient buffer size to handle all concurrent operations (
numGoroutines*numOperationsPerGoroutine). This prevents deadlocks that could occur if the channel were unbuffered or had insufficient capacity.
220-224: Proper channel initialization for multiple rollups testThe batch submission channel is now correctly initialized with a buffer size of 3, matching the number of rollups being tested. This prevents potential deadlocks when submitting batches.
265-268: Consistent channel initialization patternThe batch overwrite test now follows the same pattern of explicitly initializing the batch submission channel, making the test setup consistent across all test cases.
2 participants
Fixes Deadlock in test
Overview
Summary by CodeRabbit