-
Notifications
You must be signed in to change notification settings - Fork 14
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: common method for building the app #671
Conversation
WalkthroughThe changes introduce a new test suite for the application database in Changes
Poem
Tip We have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. 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
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
app/app_db_test.go (1)
12-22
: Implement the actual test logic and remove the skip.The test function is well-structured and follows the common pattern for setting up a test. However, there are a couple of suggestions:
- The test is being skipped using
helpers.SkipTest
. Please remove the skip once the test is implemented.- The test is not performing any actual testing, as indicated by the placeholder comment
// do something ...
. Please implement the actual test logic.app/upgrade_test.go (1)
75-89
: LGTM with a minor suggestion!The
buildApp
function correctly initializes the application and its dependencies. The encapsulation of the setup code enhances the organization of the test code and promotes code reuse.Suggestion: Add error handling for the
app.New
call.Consider adding error handling for the
app.New
call to ensure that any errors during application initialization are properly handled.Apply this diff to add error handling:
- myApp := app.New(log.NewFilter(log.NewTMLogger(os.Stdout), log.AllowAll()), + myApp, err := app.New(log.NewFilter(log.NewTMLogger(os.Stdout), log.AllowAll()), db, nil, false, map[int64]bool{}, home, 0, makeEncodingConfig, app.EmptyAppOptions{}, baseapp.SetChainID(chainId)) + require.NoError(t, err) return myApp, chainId
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/app_db_test.go (1 hunks)
- app/upgrade_test.go (2 hunks)
Additional comments not posted (1)
app/upgrade_test.go (1)
33-33
: LGTM!The refactoring of the application setup logic into the
buildApp
function improves code modularity and readability. The change adheres to best practices by promoting code reuse and separation of concerns.
Summary by CodeRabbit
New Features
Refactor