fix: Return an err no blob at a given height in case there is no blob
#88
Conversation
WalkthroughThe recent updates improve error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DummyDA
participant DataStore
Client->>DummyDA: GetIDs(height)
DummyDA->>DataStore: Check if height exists
alt Height exists
DataStore-->>DummyDA: Return IDs
DummyDA-->>Client: Return IDs
else Height does not exist
DummyDA-->>Client: Return error ("no blob at given height")
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
RollkitBot
requested review from
a team,
Manav-Aggarwal and
gupadhyaya
and removed request for
a team
![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, codebase verification and nitpick comments (2)
test/test_suite.go (2)
154-154: Add a comment for the exported functionNoBlobsAtHeightTest.The function
NoBlobsAtHeightTestis exported and should have a comment explaining its purpose.+// NoBlobsAtHeightTest checks that GetIDs returns an error when no blobs exist at a given height.Tools
golangci-lint
[warning] 154-154: exported: exported function NoBlobsAtHeightTest should have comment or be unexported
(revive)
GitHub Check: lint / golangci-lint
[warning] 154-154:
exported: exported function NoBlobsAtHeightTest should have comment or be unexported (revive)
139-139: Potential Issue Confirmed: Ignoring errors in concurrent test.In the
ConcurrentReadWriteTest, the error fromGetIDsis indeed ignored, which could lead to undetected issues during concurrent operations. Consider adding error handling forGetIDsto ensure the robustness of the test.
- File:
test/test_suite.go- Lines: 139
Analysis chain
Potential Issue: Ignoring errors in concurrent test.
In the
ConcurrentReadWriteTest, the error fromGetIDsis now ignored. This could lead to undetected issues during concurrent operations. Consider checking the error to ensure the robustness of the test.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if ignoring errors in `ConcurrentReadWriteTest` affects test reliability. # Test: Search for error handling in `ConcurrentReadWriteTest`. Expect: Errors should be asserted. rg --type go -A 5 'ConcurrentReadWriteTest'Length of output: 689
Script:
#!/bin/bash # Extract the full implementation of the ConcurrentReadWriteTest function to check error handling. ast-grep --lang go --pattern $'func ConcurrentReadWriteTest(t *testing.T, d da.DA) { $$$ }'Length of output: 1041
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- test/dummy.go (1 hunks)
- test/test_suite.go (3 hunks)
Additional context used
golangci-lint
test/test_suite.go
[warning] 154-154: exported: exported function NoBlobsAtHeightTest should have comment or be unexported
(revive)
GitHub Check: lint / golangci-lint
test/test_suite.go
[warning] 154-154:
exported: exported function NoBlobsAtHeightTest should have comment or be unexported (revive)
Additional comments not posted (2)
test/test_suite.go (1)
31-33: New Test Case: Verify error handling for non-existent blobs.The new test case "No blobs at a given height" correctly verifies the behavior of the
GetIDsmethod when no blobs exist at a specified height. This enhances the test coverage for error handling.test/dummy.go (1)
85-88: Improved Error Handling: Check for non-existent blobs.The
GetIDsmethod now checks if blobs exist at the given height and returns an error if not. This prevents potential runtime panics and improves the robustness of the method.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/test_suite.go (3 hunks)
Additional context used
golangci-lint
test/test_suite.go
[warning] 154-154: exported: comment on exported function NoBlobsAtHeightTest should be of the form "NoBlobsAtHeightTest ..."
(revive)
GitHub Check: lint / golangci-lint
test/test_suite.go
[warning] 154-154:
exported: comment on exported function NoBlobsAtHeightTest should be of the form "NoBlobsAtHeightTest ..." (revive)
Additional comments not posted (1)
test/test_suite.go (1)
31-33: New sub-test addition looks good.The addition of the "No blobs at a given height" sub-test aligns with the PR objectives and enhances the test suite.
yarikbratashchuk
force-pushed
the
yarik/no-blob-at-height
branch
from
c9a3094 to
698b32e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
test/test_suite.go (1)
158-158: Fix comment format for exported function.The comment for the
NoBlobsAtHeightTestfunction should follow the Go convention for exported functions.-// NoBlobsAtHeightTest tests the case when there are no blobs at a given height in DA +// NoBlobsAtHeightTest tests the case when there are no blobs at a given height in DA.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- test/dummy.go (2 hunks)
- test/test_suite.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/dummy.go
Additional comments not posted (2)
test/test_suite.go (2)
32-34: Addition of new test case approved.The new test case "No blobs at a given height" enhances test coverage for handling scenarios where no blobs exist at a specified height.
141-143: Verify the impact of modified error handling.The change in error handling to ignore errors unless they relate to
ErrNoBlobAtHeightmight allow other errors to go undetected. Verify that this change does not compromise the robustness of error detection during concurrent operations.
|
@tzdybal as these same fixtures used for proxy test and for direct test, i found that the easiest is to work with strings to identify specific error |
tuxcanfly
force-pushed
the
yarik/no-blob-at-height
branch
from
4ea4c65 to
d577928
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- test/dummy.go (2 hunks)
- test/test_suite.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- test/dummy.go
- test/test_suite.go
Summary by CodeRabbit
New Features
Tests