test(ut): added tests to check the existing behaviour for block/manager.go - #1550
Conversation
WalkthroughThis update focuses on enhancing the robustness of block submission testing in a blockchain context, particularly addressing issues related to block size and marshalling errors. By expanding test coverage in Changes
Assessment against linked issues
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 Configration File (
|
MSevey
left a comment
There was a problem hiding this comment.
Thanks for picking up this issue!
I left some comments to update the tests to match the desired state.
There is a good change that once written properly, the tests will actually fail. This is one of the goals of the issue. If the tests are failing, we will need to investigate the changes required to the code itself to handle those edge cases and/ore discuss internally what the right direction to go is.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- block/manager_test.go (2 hunks)
- da/da.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- block/manager_test.go
Additional comments: 4
da/da.go (4)
- 97-100: The declaration of
submittedandbreakErrvariables is clear and follows good naming conventions. However, it's important to ensure thatbreakErris only used in scenarios where breaking the loop is necessary due to an error, which seems to be the case here.- 104-106: Logging the error when failing to marshal a block is a good practice for debugging. However, wrapping the original error with additional context before breaking the loop enhances error handling by providing more detailed information about the failure.
- 110-110: Setting
breakErrtoErrBlobSizeOverLimitwhen the blob size limit is reached and then breaking the loop is an effective way to handle size constraints. This approach ensures that the method does not attempt to submit blobs that exceed the maximum allowed size, which is a good practice for maintaining system stability.- 117-121: The check for
submitted == 0 && breakErr != nilbefore returning an error is a logical way to handle cases where no blocks were submitted due to an error. This ensures that the method returns a meaningful error message in such scenarios, improving the error handling mechanism.
@MSevey can you please have a look again when you find time? |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- block/manager_test.go (2 hunks)
Additional comments: 2
block/manager_test.go (2)
- 230-271: The test cases added for submitting blocks to the DA component cover the scenarios outlined in the PR objectives well. However, it's important to ensure that the
isErrExpectedflag accurately reflects the expected outcome of each test case. For instance, in the case where a block is too big on its own, it might be beneficial to verify that the error thrown is specifically related to the size limit. This could involve checking the error message or type to ensure it's what's expected.Consider adding assertions to verify the specific type or message of the expected errors in test cases where
isErrExpectedistrue. This will ensure that the tests are failing for the right reasons and improve the robustness of the test suite.
- 267-267: As per the existing comment from
MSevey, it's crucial to callinvalidateBlockHeaderon block A in the relevant test case to simulate a marshalling error. This action has been correctly implemented in the test case "A itself has a marshalling error. So A, B and C never get submitted", ensuring that the test setup aligns with the intended scenario.
MSevey
left a comment
There was a problem hiding this comment.
Alright this is great. let's update the test case to have it pass and then I'll create some follow up issues to improve how these test cases are handled. At least now the full functionality is documented in these tests.
|
Hey @arhamj, thanks for submitting this! It seems like the first test you added is failing in the unit test, could you make sure it's passing? |
@Manav-Aggarwal I had added a fix to handle the failing case, but @MSevey asked me to revert it to see what the failures were. |
@arhamj yes and then my last review was to update the test so that it passes with the current code. |
|
Sorry, I missed it completely @MSevey 😄. Updating. |
|
Hey, sorry for the delay. @MSevey I think the marshalling error tests no longer make sense as we have gotten rid of the in-memory queue for the blocks. The store.SaveBlock() to add data to the DB takes care of MarshalErrors when writing and if there were a read error, store.GetBlock() would catch it. I've updated the PR. Let me know what you think. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1550 +/- ##
==========================================
+ Coverage 50.23% 52.10% +1.87%
==========================================
Files 52 52
Lines 6762 5600 -1162
==========================================
- Hits 3397 2918 -479
+ Misses 3024 2345 -679
+ Partials 341 337 -4 ☔ View full report in Codecov by Sentry. |
tzdybal
left a comment
There was a problem hiding this comment.
This new test case makes sense 👍
tzdybal
left a comment
There was a problem hiding this comment.
Please remove the unused function.
we should add this test back in. the reason is that we want to test functions in isolation. |
…r_test.go refactor(manager): update test cases and refactored da submission method fix(revert): da changes to observe breaking test cases
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- Makefile (1 hunks)
- block/manager_test.go (3 hunks)
- test/mocks/Store.go (1 hunks)
Check Runs (4)
lint / hadolint / hadolint completed (1)
lint / golangci-lint completed (1)
lint / markdown-lint completed (1)
lint / yamllint completed (1)
Files skipped from review due to trivial changes (1)
- test/mocks/Store.go
Files skipped from review as they are similar to previous changes (1)
- block/manager_test.go
Additional comments: 1
Makefile (1)
- 87-87: The addition of the
mock-gencommand for generating mocks for theStoreentity is correctly implemented and follows the established pattern for mock generation in this Makefile.Please ensure to verify the successful generation of mocks and their integration within the testing framework.
MSevey
left a comment
There was a problem hiding this comment.
Please revert back to just the new test cases to TestSubmitBlocksToDA. We don't need the new mocks or the separate tests. Only the new test cases.
Overview
This MR has changes which potentially add the missing tests to
manager_test.go. Currently, the tests only validate the existing behaviours, i.e.:Closes: #1530
From the code, it looks like marshal errors would be caught earlier at
SaveBlockto the store stage. That said I would like the following change:da.gotomanager.goto avoid retries in cases where there are such issues and also for better error logging.Checklist
Summary by CodeRabbit
mockerycommand for generating mocks of theStoreentity to facilitate testing.Storetype to simulate store interactions in tests.