Uh oh!
There was an error while loading. Please reload this page.
chore: quick cleanup - #2239
Conversation
WalkthroughThis set of changes primarily focuses on code modernization and cleanup across multiple packages. The main updates include replacing the use of Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant TestFunction
participant MockComponent
Tester->>TestFunction: Call test with t.Context()
TestFunction->>MockComponent: Use t.Context() in mock expectations
TestFunction->>TestFunction: Run test logic (unchanged)
TestFunction->>MockComponent: Call method with t.Context()
MockComponent-->>TestFunction: Return expected result
TestFunction-->>Tester: Assert results
Suggested labels
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).
|
Uh oh!
There was an error while loading. Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #2239 +/- ##
==========================================
+ Coverage 55.01% 55.02% +0.01%
==========================================
Files 52 52 Lines 4508 4507 -1 ==========================================
Hits 2480 2480 + Misses 1762 1761 -1
Partials 266 266 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR performs a quick cleanup to update context-usage and parameter types from interface{} to any, while also replacing traditional for-loops with the “for range” construct throughout several test and helper files.
- Update of context propagation to use t.Context() instead of manually creating a context
- Replacement of “for i := 0; i < X; i++” loops with “for range X” loops across multiple test files
- Substitution of interface{} with any in several function signatures to reflect updated coding style
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| types/serialization_test.go | Changed loop syntax by replacing index-based loop with “for range 8”. |
| test/e2e/sut_helper.go | Updated context usage and replaced traditional loop with “for range 5”. |
| pkg/store/store_test.go | Switched context and loop syntax in tests using constant “n”. |
| pkg/store/prefix_test.go | Revised loops to use “for range len(keys)” instead of index-based loops. |
| pkg/signer/file/file_signer_test.go | Updated loops to use “for range” on integer variables numGoRoutines and messageCount. |
| pkg/p2p/utils_test.go | Modified loop syntax to use “for range” on integer n. |
| pkg/cache/cache_test.go | Changed loops to use “for range” on integer values goroutines and operations. |
| node/full_node_integration_test.go | Converted several index-based loops to “for range” style. |
| ... | Additional similar adjustments in remaining files. |
| // create random hashes | ||
| h := []Hash{} | ||
| for i := 0; i < 8; i++ { | ||
| for range 8 { |
There was a problem hiding this comment.
Using 'for range' directly on an integer constant is not valid in Go as integers are not iterable. Consider using 'for i := 0; i < 8; i++' or 'for range make([]struct{}, 8)' if an index is not needed.
| forrange8 { | |
| forrangemake([]struct{}, 8) { |
| }() | ||
| }) | ||
| for i := 0; i < 5; i++ { | ||
| for range 5 { |
There was a problem hiding this comment.
Using 'for range' on an integer literal is not valid in Go; use a conventional loop like 'for i := 0; i < 5; i++' or iterate over a slice of length 5 instead.
| forrange5 { | |
| fori:=0; i<5; i++ { |
| for i := range n { | ||
| require.NoError(s.SetMetadata(t.Context(), getKey(i), getValue(i))) | ||
| } | ||
| for i := 0; i < n; i++ { | ||
| value, err := s.GetMetadata(ctx, getKey(i)) | ||
| for i := range n { |
There was a problem hiding this comment.
The 'for range' construct cannot be used directly on an integer (n); use a loop like 'for i := 0; i < n; i++' or range over a slice of length n.
| for i := 0; i < len(keys); i++ { | ||
| err := prefixbatchkv1.Put(ctx, keys[i], values[i]) | ||
| for i := range len(keys) { |
There was a problem hiding this comment.
Ranging over the result of 'len(keys)' is invalid because len(keys) returns an integer. Consider using 'for i := 0; i < len(keys); i++' or 'for i := range keys' instead.
| fori:=rangelen(keys) { | |
| fori:=0; i<len(keys); i++ { |
| for i := range numGoRoutines { | ||
| go func(routineNum int) { | ||
| for j := 0; j < messageCount; j++ { | ||
| for j := range messageCount { |
There was a problem hiding this comment.
The expression 'numGoRoutines' is an integer; using 'for range' on an integer is invalid. Use 'for i := 0; i < numGoRoutines; i++' instead.
| for i := range numGoRoutines { | ||
| go func(routineNum int) { | ||
| for j := 0; j < messageCount; j++ { | ||
| for j := range messageCount { |
There was a problem hiding this comment.
Using 'for range' on 'messageCount', which is an integer, is not allowed. Replace with a conventional loop such as 'for j := 0; j < messageCount; j++'.
| for i := range goroutines { | ||
| go func(id int) { | ||
| defer wg.Done() | ||
| for j := 0; j < operations; j++ { | ||
| for j := range operations { |
There was a problem hiding this comment.
The variable 'goroutines' is an integer and cannot be directly iterated over using 'for range'. Consider replacing with 'for i := 0; i < goroutines; i++'.
| for i := range goroutines { | ||
| go func(id int) { | ||
| defer wg.Done() | ||
| for j := 0; j < operations; j++ { | ||
| for j := range operations { |
There was a problem hiding this comment.
Since 'operations' is an integer, iterating over it with 'for range' is invalid. Use a traditional loop syntax like 'for j := 0; j < operations; j++' instead.
| mnet := mocknet.New() | ||
| for i := 0; i < n; i++ { | ||
| for i := range n { |
There was a problem hiding this comment.
The identifier 'n' is an integer and cannot be used with 'for range' directly. Replace with a loop such as 'for i := 0; i < n; i++' or range over a constructed slice.
| fori:=rangen { | |
| fori:=0; i<n; i++ { |
| var err error | ||
| m.logger.Debug("trying to retrieve block from DA", "daHeight", daHeight) | ||
| for r := 0; r < dAFetcherRetries; r++ { | ||
| for range dAFetcherRetries { |
There was a problem hiding this comment.
Using 'for range' on 'dAFetcherRetries' (an integer) is not valid. Consider using 'for r := 0; r < dAFetcherRetries; r++' instead.
| forrangedAFetcherRetries { | |
| forr:=0; r<dAFetcherRetries; r++ { |
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
pkg/store/prefix_test.go (1)
100-104:⚠️ Potential issueSame scalar-range issue in verification loop
Repeat of the previous compile error; apply the same correction.
- for i := range len(keys) {+ for i := range keys {
🧹 Nitpick comments (1)
block/publish_block_test.go (1)
309-309: Consider cleanup of unused cancel functionYour change to use t.Context() means the cancel function returned from setupManagerForPublishBlockTest is no longer used. Consider a follow-up PR to update the test setup function to no longer return the now-unused cancel function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
block/publish_block_test.go(2 hunks)block/reaper_test.go(4 hunks)block/retriever.go(1 hunks)block/submitter.go(1 hunks)node/full_node_integration_test.go(4 hunks)node/helpers.go(1 hunks)pkg/cache/cache_test.go(1 hunks)pkg/cmd/run_node_test.go(1 hunks)pkg/config/config_test.go(1 hunks)pkg/config/yaml.go(1 hunks)pkg/genesis/io_test.go(1 hunks)pkg/os/os.go(1 hunks)pkg/os/os_test.go(1 hunks)pkg/p2p/client_test.go(2 hunks)pkg/p2p/utils_test.go(3 hunks)pkg/signer/file/file_signer_test.go(1 hunks)pkg/store/prefix_test.go(2 hunks)pkg/store/store_test.go(5 hunks)test/e2e/sut_helper.go(3 hunks)types/serialization_test.go(1 hunks)types/utils.go(1 hunks)types/utils_test.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
node/helpers.go
[warning] 37-37: node/helpers.go#L37
Added line #L37 was not covered by tests
[warning] 40-40: node/helpers.go#L40
Added line #L40 was not covered by tests
block/submitter.go
[warning] 113-113: block/submitter.go#L113
Added line #L113 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (32)
pkg/genesis/io_test.go (1)
97-97: Approve modernanyalias usage. Replacinginterface{}withanyimproves readability and aligns with Go 1.18+ idioms.pkg/p2p/client_test.go (2)
85-85: Uset.Context()for test context. Leveragingt.Context()simplifies test setup and ensures context cancellation with the test lifecycle.
106-106: Uset.Context()for test context. Consistent with modern Go testing patterns and the other test file updates.pkg/p2p/utils_test.go (1)
60-61: Improved slice copying approach.The change from using
append(net.IP{}, unicastAddr...)to explicitly allocating memory withmakefollowed bycopyis a good optimization. This approach is more direct and potentially more efficient by pre-allocating the exact size needed.pkg/config/config_test.go (1)
279-279: Good modernization of type signature.Changing from
interface{}toanyfollows modern Go conventions (Go 1.18+) and improves code readability while maintaining the same functionality. This is a recommended practice.node/full_node_integration_test.go (4)
148-153: Loop syntax updated to use range-based iteration.The loop has been updated to use the more idiomatic Go range-based iteration. This change maintains the same behavior as the original code while making it more concise.
239-245: Loop syntax updated to use range-based iteration with index.The loop has been converted to use range-based iteration. The index variable is still needed here since it's used for logging, which is correctly preserved in the new syntax.
255-263: Loop syntax updated to use range-based iteration with index.The loop has been converted to use range-based iteration while preserving the index variable needed for logging. This maintains the same functionality with more idiomatic Go syntax.
291-304: Loop syntax updated to use range-based iteration with index.The loop has been converted to use range-based iteration while preserving the index variable that's needed for logging. This change follows modern Go conventions while maintaining the same behavior.
pkg/store/store_test.go (11)
43-44: Context management improved using t.Context().Test code has been improved by using the built-in test context instead of creating a manual context. This is a best practice in Go testing that simplifies context management.
49-52: Context management improved using t.Context().Test context handling has been improved by using the built-in test context for DB operations, following Go testing best practices.
55-56: Context management improved using t.Context().Using the test's built-in context for the
Height()call improves test code by eliminating manual context management.
100-101: Context management improved using t.Context().Using the test's built-in context for the
SaveBlockData()call improves test code by eliminating manual context management.
106-107: Context management improved using t.Context().Using the test's built-in context for the
GetBlockData()call improves test code by eliminating manual context management.
113-114: Context management improved using t.Context().Using the test's built-in context for the
GetSignature()call improves test code by eliminating manual context management.
135-138: Context management improved using t.Context().Using the test's built-in context for the
UpdateState()call improves test code by eliminating manual context management.
149-151: Context management improved using t.Context().Using the test's built-in context for the
GetState()call improves test code by eliminating manual context management.
174-176: Loop syntax updated to use range-based iteration.The loop has been updated to use the more idiomatic Go range-based iteration. Since the loop only uses the index variable to generate keys and values, this change maintains the same behavior.
178-182: Loop syntax updated and context management improved.The loop syntax has been updated to use range-based iteration, and the context for
GetMetadata()has been changed to use the test's built-in context. Both changes follow Go best practices without altering behavior.
184-186: Context management improved using t.Context().Using the test's built-in context for the
GetMetadata()call improves test code by eliminating manual context management.test/e2e/sut_helper.go (4)
154-158: Type declaration updated frominterface{}toany.The parameter type has been updated to use the more modern
anytype, which is an alias forinterface{}introduced in Go 1.18. This improves code readability without changing functionality.
160-164: Type declaration updated frominterface{}toany.The parameter type has been updated to use the more modern
anytype, which is an alias forinterface{}introduced in Go 1.18. This improves code readability without changing functionality.
185-187: Updated variadic parameter type frominterface{}toany.The variadic parameter type has been updated to use the more modern
anytype, which is an alias forinterface{}introduced in Go 1.18. This improves code readability without changing functionality.
218-223: Loop syntax updated to use range-based iteration.The loop in the
Shutdownmethod has been updated to use the more idiomatic Go range-based iteration. Since the loop body doesn't use the index variable, this change maintains the same behavior while making the code more concise.pkg/cmd/run_node_test.go (1)
102-103: Updated struct field types frominterface{}toany.The struct field types have been updated to use the more modern
anytype, which is an alias forinterface{}introduced in Go 1.18. This improves code readability without changing functionality.pkg/os/os.go (1)
13-13: Modernize variadic type alias for consistencyReplacing
...interface{}with...anyis a safe, idiomatic update for Go 1.18+. No functional change.node/helpers.go (1)
37-37: Update variadic parameters to useanyChanging signatures from
...interface{}to...anyaligns with Go 1.18+ idioms without altering behavior. These helper methods remain no-op stubs and maintain expected signatures.Also applies to: 40-40
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-37: node/helpers.go#L37
Added line #L37 was not covered by testspkg/os/os_test.go (1)
13-13: Synchronize mock logger with updated interfaceUpdating
lastKVto[]anyandInfo(...any)keeps the mock in sync with theloggerinterface. No logic changes.Also applies to: 16-16
block/reaper_test.go (1)
28-28: Uset.Context()for consistent test contextSwitching to the built-in
t.Context()cleans up manual context setup and cancellation in tests, while preserving existing test logic.Also applies to: 63-63, 96-96, 113-113
block/publish_block_test.go (3)
312-323: Great improvement using t.Context()Replacing the manually created context with t.Context() improves the code by:
- Better aligning with modern Go testing conventions
- Reducing boilerplate context management code
- Ensuring the test context is properly propagated
Since you've consistently applied this change to all mock expectations, this is a clean improvement.
329-330: Consistent context use in executor mocksGood job maintaining consistency by updating these executor mock expectations to also use t.Context().
345-346: Clean sequencer mock and manager call updatesThe sequencer mock and the publishBlock call have been properly updated to use t.Context(), completing the conversion from manual context to test framework context.
Uh oh!
There was an error while loading. Please reload this page.
| // Generate 100 random transactions and check that they are all unique | ||
| txSet := make(map[string]bool) | ||
| for i := 0; i < 100; i++ { | ||
| for range 100 { |
There was a problem hiding this comment.
Invalid loop syntax over integer. for range 100 is invalid since range requires a collection (slice, array, map, or channel). Replace with a traditional for-loop.
Proposed fix:
- for range 100 {+ for i := 0; i < 100; i++ {📝 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.
| forrange100 { | |
| fori:=0; i<100; i++ { |
Uh oh!
There was an error while loading. Please reload this page.
| for i := range goroutines { | ||
| go func(id int) { | ||
| defer wg.Done() | ||
| for j := 0; j < operations; j++ { | ||
| for j := range operations { | ||
| // Test concurrent item operations |
There was a problem hiding this comment.
Range-over-integer causes compilation failure
range expects an iterable (array, slice, map, string, channel).
Here both goroutines and operations are plain int constants, so the test will not compile (cannot range over <int>).
- for i := range goroutines {+ for i := 0; i < goroutines; i++ {
…
- for j := range operations {+ for j := 0; j < operations; j++ {📝 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.
| fori:=rangegoroutines { | |
| gofunc(idint) { | |
| deferwg.Done() | |
| forj:=0; j<operations; j++ { | |
| forj:=rangeoperations { | |
| // Test concurrent item operations | |
| fori:=0; i<goroutines; i++ { | |
| gofunc(idint) { | |
| deferwg.Done() | |
| forj:=0; j<operations; j++ { | |
| // Test concurrent item operations |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/cache/cache_test.go (1)
102-102:⚠️ Potential issueRange-over-integer causes compilation failure
Go's
rangekeyword can only iterate over arrays, slices, maps, strings, or channels. Sincegoroutinesis an integer constant (defined as10on line 96), usingrange goroutineswill cause a compilation error.- for i := range goroutines {+ for i := 0; i < goroutines; i++ {
🧹 Nitpick comments (1)
pkg/cache/cache_test.go (1)
105-105: Inconsistent loop styles within the same functionWhile the outer loop was changed to use
range(incorrectly), the inner loop still uses the traditionalfor j := 0; j < operations; j++style. For consistency, either both loops should use the same style, or both should be updated correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/cache/cache_test.go(1 hunks)pkg/p2p/utils_test.go(2 hunks)pkg/store/prefix_test.go(2 hunks)pkg/store/store_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/store/prefix_test.go
- pkg/p2p/utils_test.go
- pkg/store/store_test.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: test / Run Unit Tests
- GitHub Check: Analyze (go)
- GitHub Check: Summary
Overview
Summary by CodeRabbit