-
Notifications
You must be signed in to change notification settings - Fork 342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add tests for governance modifiable params #2973
test: add tests for governance modifiable params #2973
Conversation
…#2974) ## Overview Addresses #2966 by using the governance modifiable params tests in PR #2973. All four of the params are modifiable by governance, which matches the specs in [params.md](https://github.com/celestiaorg/celestia-app/blob/main/specs/src/specs/params.md). ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords
WalkthroughWalkthroughThe recent updates involve a refinement in the blockchain parameters, particularly for the Celestia application. The Changes
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? TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! This mostly LGTM.
suite.Run(t, new(TestSuite)) | ||
} | ||
|
||
func (suite *TestSuite) TestModifiableParameters() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] do we want another test for TestUnmodifiableParameters()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm currently doing that in PR #2919. I think the code is more organized & readable if we do have separate function tests for modifiable and unmodifiable params. Would love to get your input on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea two separate tests works for me. IMO we shouldn't merge #2919 until we get rough alignment via the CIP process so I think it could make sense to port the TestUnmodifiableParameters()
test here.
- One PR for increasing test coverage.
- If we deem it necessary, a separate PR for implementation changes. This second PR will be more confidence-inspiring if we also have to update tests and move some params from
TestModifiableParameters
toTestUnmodifiableParameters
@fahimahmedx can you port the |
Closing in favor of #3007. Thanks for all your work on this! |
Overview
The second PRs that addresses #2916. This PR adds test cases for the governance modifiable params.
Checklist