chore(client): use command's configured output #22334
Conversation
I have some external tests that invoke some of the SDK's cobra commands. Those tests often involve creating the command, calling cmd.SetOutput, executing the command, and then inspecting its output. This change updates the client context to prefer cmd.Output if set (which should only happen in tests), falling back to stdout otherwise; so it should not change any existing client-facing behavior, but it does otherwise make command output testable.
mark-rushakoff
requested review from
julienrbrt,
JulianToledano and
a team
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Suggested labels
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (1)
client/cmd_test.go (1)
142-165: Consider enhancing test coverageWhile the current test case is good, consider adding:
- Test documentation explaining the test's purpose
- Negative test cases (e.g., when PrintString returns an error)
- Additional test cases with different output content
Example enhancement:
+// TestContext_usesCobraCommandOutput verifies that the client context correctly +// uses the cobra command's configured output when printing strings. func TestContext_usesCobraCommandOutput(t *testing.T) { var initCtx client.Context + + testCases := []struct { + name string + outputContent string + expectedError bool + }{ + {"basic output", "hello", false}, + {"empty output", "", false}, + {"multiline output", "hello\nworld", false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { cmd := &cobra.Command{ PreRunE: func(cmd *cobra.Command, args []string) error { return client.SetCmdClientContextHandler(initCtx, cmd) }, RunE: func(cmd *cobra.Command, _ []string) error { cctx, err := client.GetClientTxContext(cmd) if err != nil { return err } - return cctx.PrintString("hello") + return cctx.PrintString(tc.outputContent) }, } var outBuf bytes.Buffer cmd.SetOutput(&outBuf) require.NoError(t, cmd.Execute()) - require.Equal(t, "hello", outBuf.String()) + require.Equal(t, tc.outputContent, outBuf.String()) + }) + } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- client/cmd.go (1 hunks)
- client/cmd_test.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
client/cmd.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/cmd_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (3)
client/cmd_test.go (2)
4-4: LGTM: Import addition is appropriateThe
bytespackage import is correctly placed and necessary for the new test implementation.
142-165: LGTM: Test implementation is well-structuredThe test correctly verifies the command output functionality with proper setup and assertions.
client/cmd.go (1)
348-348: LGTM! Good improvement for testability.The change to use
cmd.OutOrStdout()is a good improvement that:
- Enhances testability by respecting the command's configured output
- Maintains backward compatibility by falling back to stdout when no output is configured
- Follows best practices by utilizing cobra's built-in methods
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- client/cmd.go (1 hunks)
- client/cmd_test.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
client/cmd.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/cmd_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (2)
client/cmd_test.go (1)
4-4: LGTM: Import follows Go conventionsThe new
bytesimport is correctly grouped with standard library imports.client/cmd.go (1)
348-348: LGTM! The change enhances testability while maintaining backward compatibility.The modification correctly uses
cmd.OutOrStdout()to respect the command's configured output while defaulting to stdout, which aligns with the PR objectives of improving testing capabilities without affecting existing behavior.Let's verify that the change doesn't affect existing command output behavior:
✅ Verification successful
Let me analyze the results and generate additional verification scripts to ensure we have a complete understanding of the impact.
The change is safe and correctly implements output handling
Based on the verification results:
- The
WithOutputimplementation inclient/context.gois straightforward and correctly sets the output writer- Existing tests already use
cmd.SetOutput()for controlling command output in test scenarios- The change aligns with existing patterns where tests need to capture or discard output
- All usages of
GetClientTxContextacross the codebase follow the same pattern and will benefit from this enhancementThe modification maintains backward compatibility since:
cmd.OutOrStdout()defaults to stdout when no custom output is set- The
WithOutputmethod simply updates the context without side effects- Existing test patterns demonstrate this is a common and safe approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that commands still output to stdout by default # and that the output can be customized in tests. # Test 1: Search for test files that verify command output behavior rg -g '**/*_test.go' -l 'cmd\.SetOutput' # Test 2: Search for any direct usage of GetClientTxContext # to ensure we're not breaking any existing patterns rg 'GetClientTxContext\(' --type goLength of output: 2984
Script:
#!/bin/bash # Let's examine the test files that use SetOutput to understand # how the output is being tested and verify our change aligns with existing patterns # Check test implementations rg -g '**/*_test.go' -B 3 -A 3 'cmd\.SetOutput' # Also check the implementation of WithOutput to ensure # it's properly handling the output configuration ast-grep --pattern 'func (ctx $_) WithOutput($_) $_'Length of output: 3715
| func TestContext_usesCobraCommandOutput(t *testing.T) { | ||
| var initCtx client.Context | ||
|
|
||
| cmd := &cobra.Command{ | ||
| PreRunE: func(cmd *cobra.Command, args []string) error { | ||
| return client.SetCmdClientContextHandler(initCtx, cmd) | ||
| }, | ||
| RunE: func(cmd *cobra.Command, _ []string) error { | ||
| cctx, err := client.GetClientTxContext(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return cctx.PrintString("hello") | ||
| }, | ||
| } | ||
|
|
||
| var outBuf bytes.Buffer | ||
| cmd.SetOutput(&outBuf) | ||
|
|
||
| require.NoError(t, cmd.Execute()) | ||
|
|
||
| require.Equal(t, "hello", outBuf.String()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider expanding test coverage with additional test cases
The test effectively validates the basic functionality of using command's configured output. However, consider enhancing it with the following improvements:
- Add test cases for when
cmd.Outputis not set to verify default stdout behavior - Include error scenarios (e.g., when context setup fails)
- Convert to table-driven tests to cover multiple scenarios
- Add documentation explaining the test's purpose and scenarios covered
Here's a suggested enhancement:
+// TestContext_usesCobraCommandOutput verifies that the client context correctly
+// uses the command's configured output when set, and falls back to stdout when not set.
func TestContext_usesCobraCommandOutput(t *testing.T) {
- var initCtx client.Context
-
- cmd := &cobra.Command{
+ testCases := []struct {
+ name string
+ setupOutput bool
+ expectedOut string
+ expectedError error
+ }{
+ {
+ name: "with configured output",
+ setupOutput: true,
+ expectedOut: "hello",
+ },
+ {
+ name: "without configured output",
+ setupOutput: false,
+ expectedOut: "hello",
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ var initCtx client.Context
+ cmd := &cobra.Command{Committable suggestion was skipped due to low confidence.
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 1c949f7)
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
Description
I have some external tests that invoke some of the SDK's cobra commands. Those tests often involve creating the command, calling cmd.SetOutput, executing the command, and then inspecting its output.
This change updates the client context to prefer cmd.Output if set (which should only happen in tests), falling back to stdout otherwise; so it should not change any existing client-facing behavior, but it does otherwise make command output testable.
Summary by CodeRabbit
New Features
Tests