test: Audit some integration tests, make minor changes, and add plaintext test list #2089
Conversation
github-actions
bot
added
C:Testing
github-actions
bot
added
the
C:x/provider
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughWalkthroughThe recent updates introduce a comprehensive GitHub Actions workflow to maintain testing documentation, enhance Makefile functionality for documentation testing, and significantly improve integration test documentation. Multiple new tests have been added to validate key functionalities, particularly related to blockchain operations, including rewards distribution, validator behavior, and misbehavior detection. These enhancements ensure better clarity in documentation and robust testing coverage, ultimately improving the reliability and maintainability of the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHubActions as GA
participant Makefile as MF
participant Docs as D
participant Tests as T
Developer->>GA: Trigger events (PR, push)
GA->>MF: Execute testing-docs workflow
MF->>T: Run tests on integration changes
GA->>D: Update documentation if needed
D-->>Developer: Notify documentation status
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range, codebase verification and nitpick comments (13)
.github/workflows/testing-docs.yml (1)
42-52: Step for checking for changes is appropriate but consider improving error messaging.The step correctly compares the generated documentation with the committed version and exits with an error if they differ. Consider adding more descriptive error messages for better clarity.
- echo "Documentation for integration tests is out of date. Please run `make testing-docs`!" + echo "Error: Documentation for integration tests is out of date. Please run `make testing-docs` to update it."scripts/test_doc/extract_docstrings.go (1)
64-79: Docstring extraction logic is appropriate but consider usingos.ReadFile.The
extractDocstringsfunction reads the Go source file, creates the AST, and traverses it to extract docstrings. This logic is appropriate. However, consider usingos.ReadFileinstead ofioutil.ReadFileasioutilis deprecated.- src, err := ioutil.ReadFile(filePath) + src, err := os.ReadFile(filePath)tests/integration/slashing.go (8)
Line range hint
1-1:
Ensure consistent import aliasing.The import alias for
evidencetypesis redundant since it matches the package name.- evidencetypes "cosmossdk.io/x/evidence/types" + "github.com/cosmos/cosmos-sdk/x/evidence/types"
Line range hint
53-53:
Check for nil error instead of usingNoError.Using
NoErroris fine, but explicitly checking for nil error is more readable in some cases.- s.Require().Nil(err) + s.Require().NoError(err)
Line range hint
155-155:
Check for nil error instead of usingNoError.Using
NoErroris fine, but explicitly checking for nil error is more readable in some cases.- s.Require().Nil(err) + s.Require().NoError(err)
258-262: Improve the function description.The long description placeholder should be replaced with a detailed explanation of the test's purpose.
-// @Long Description@ +// This test ensures that a slash packet sent from a consumer chain is properly acknowledged by the provider chain.
Line range hint
320-320:
Check for nil error instead of usingNoError.Using
NoErroris fine, but explicitly checking for nil error is more readable in some cases.- suite.Require().Nil(err) + suite.Require().NoError(err)
Line range hint
388-388:
Check for nil error instead of usingNoError.Using
NoErroris fine, but explicitly checking for nil error is more readable in some cases.- suite.Require().Nil(err) + suite.Require().NoError(err)
Line range hint
571-571:
Check for nil error instead of usingNoError.Using
NoErroris fine, but explicitly checking for nil error is more readable in some cases.- suite.Require().Nil(err) + suite.Require().NoError(err)
Line range hint
725-725:
Check for nil error instead of usingNoError.Using
NoErroris fine, but explicitly checking for nil error is more readable in some cases.- suite.Require().Nil(err) + suite.Require().NoError(err)tests/integration/distribution.go (3)
23-31: Improve the function description.The long description placeholder should be replaced with a detailed explanation of the test's purpose.
-// @Long Description@ +// This test verifies the correct distribution of rewards from the consumer chain to the provider chain. +// It sets up the necessary channels, sends tokens to the fee pool, and checks their distribution. +// The test ensures that tokens are correctly allocated to the consumer and provider chains, +// and verifies the handling of rewards that are not whitelisted.
194-200: Improve the function description.The long description placeholder should be replaced with a detailed explanation of the test's purpose.
-// @Long Description@ +// This test verifies the retry mechanism for failed reward transmissions. +// It sets up the necessary channels, fills the fee pool, and intentionally corrupts the transmission channel. +// The test then advances the block height to trigger a retry and confirms the successful transmission of rewards.
850-860: Improve the function description.The long description placeholder should be replaced with a detailed explanation of the test's purpose.
-// @Long Description@ +// This test verifies the allocation of tokens to consumer validators under various scenarios. +// It sets up the provider chain, initializes consumer validators, and calls the AllocateTokensToConsumerValidators function. +// The test checks the expected token allocation to validators and covers different scenarios such as empty tokens, +// empty consumer validator set, single validator allocation, and multiple validator allocation.
p-offtermatt
changed the title
|
@coderabbitai pause |
|
@coderabbitai resolve |
There was a problem hiding this comment.
Great work @p-offtermatt @kirdatatjana. See my comments below. Some of them, like removing certain tests, might be out of the scope of this PR.
| @@ -634,6 +680,11 @@ func (suite *CCVTestSuite) TestValidatorDoubleSigning() { | |||
|
|
|||
| // TestQueueAndSendSlashPacket tests the integration of QueueSlashPacket with SendPackets. | |||
There was a problem hiding this comment.
Why is this not tested in an UT?
There was a problem hiding this comment.
I think it could be written as a unit test.
| @@ -714,6 +765,10 @@ func (suite *CCVTestSuite) TestQueueAndSendSlashPacket() { | |||
| // TestCISBeforeCCVEstablished tests that the consumer chain doesn't panic or | |||
There was a problem hiding this comment.
ditto: Why not test this in a UT?
There was a problem hiding this comment.
I think this could potentially be written as a unit test, but since it involves processing multiple blocks on the consumer chain, it's more practical to implement it as an integration test.
| @@ -23,6 +23,7 @@ import ( | |||
| const stakeMultiplier = 1000000 | |||
|
|
|||
| // TestMinStake tests the min stake parameter. | |||
There was a problem hiding this comment.
Is this testing relaying of the validator updates or the creating of the consumer validator set on the provider chain? The later doesn't require integration tests.
There was a problem hiding this comment.
It does both, so I believe it should stay.
Co-authored-by: Marius Poke <marius.poke@posteo.de>
Co-authored-by: Marius Poke <marius.poke@posteo.de>
Co-authored-by: Marius Poke <marius.poke@posteo.de>
Co-authored-by: Marius Poke <marius.poke@posteo.de>
Description
Adds a script that generates a nice list of scenarios that are tested via integration tests.
Added missing docstrings and improved existing ones for several tests, along with a few minor fixes.
Extra notes:
This function interchain-security/testutil/keeper/unit_test_helpers.go
should use rapid instead. With rand, we only get one attempt at this per run, and it's impossible to trace back the original parameters if it ever fails.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
TESTING.mdto provide clearer guidelines for documenting integration tests, including schema requirements.Chores