test: add more tests to types#2352
Conversation
|
Warning Rate limit exceeded@Manav-Aggarwal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughA variable representing a namespace placeholder has been renamed and exported in the Changes
Suggested labels
Suggested reviewers
Poem
✨ 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).
|
71b1123 to
436e689
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2352 +/- ##
==========================================
+ Coverage 65.94% 67.61% +1.67%
==========================================
Files 66 66
Lines 6155 6155
==========================================
+ Hits 4059 4162 +103
+ Misses 1728 1624 -104
- Partials 368 369 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
Adds comprehensive unit tests for the DA helper functions and makes the namespace placeholder constant public.
- Introduces new test cases in
types/da_test.goforSubmitWithHelpers(success and multiple error mappings) andRetrieveWithHelpers(ID lookup and blob retrieval scenarios). - Exports the
nameSpacePlaceholderconstant asNameSpacePlaceholderand updates its usage in both submit and ID‐lookup calls.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| types/da_test.go | New test table entries for SubmitWithHelpers and RetrieveWithHelpers covering success paths and error conditions. |
| types/da.go | Renamed nameSpacePlaceholder to NameSpacePlaceholder and updated SubmitWithHelpers/GetIDs calls to use it. |
Comments suppressed due to low confidence (3)
types/da.go:116
- Replace the inline placeholder literal with the exported
NameSpacePlaceholderconstant to ensure consistency and avoid duplication.
blobs, err := da.Get(ctx, idsResult.IDs, []byte("placeholder"))
types/da.go:14
- [nitpick] Consider renaming
NameSpacePlaceholdertoNamespacePlaceholder(single word) to align with common terminology and improve readability.
var NameSpacePlaceholder = []byte("placeholder")
types/da_test.go:163
- Add an
expectedTimestampfield to the test case and assert thatresult.Timestampmatches the originalGetIDsResult.Timestampin the successful retrieval scenario.
expectedHeight: dataLayerHeight,
2a73aa5 to
ccc1691
Compare
ccc1691 to
2a73aa5
Compare
alpe
left a comment
alpe
left a comment
There was a problem hiding this comment.
👍 I added some minor comments and personal preferences
| logger := log.NewNopLogger() | ||
|
|
||
| testCases := []struct { | ||
| name string |
There was a problem hiding this comment.
personal preference: no need for name field when you use map[string]struct{} for test cases with name as key
| testCases := []struct { | ||
| name string | ||
| data [][]byte | ||
| gasPrice float64 |
There was a problem hiding this comment.
nit: gasPrices and options are always the same and can be defined in the test body
| if tc.expectedIDs != nil { | ||
| assert.Equal(t, tc.expectedIDs, result.IDs) | ||
| } | ||
| if tc.expectedCount != 0 { |
There was a problem hiding this comment.
no need for this if condition. You can always check.
it is actually len(ids) for the happy path
| if tc.expectedIDs != nil { | ||
| assert.Equal(t, tc.expectedIDs, result.IDs) | ||
| } | ||
| if tc.expectedData != nil { |
There was a problem hiding this comment.
nit: you can remove the if conditions here and L229
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. NOTE: PR titles should follow semantic commits: https://www.conventionalcommits.org/en/v1.0.0/ --> ## Overview ref #2256 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Added comprehensive unit tests for helper functions related to data submission and retrieval, covering various success and error scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: tac0turtle <you@example.com>
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. NOTE: PR titles should follow semantic commits: https://www.conventionalcommits.org/en/v1.0.0/ --> ## Overview ref #2256 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Added comprehensive unit tests for helper functions related to data submission and retrieval, covering various success and error scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: tac0turtle <you@example.com>
Overview
ref #2256
Summary by CodeRabbit