test: add cmdtest package #15251
Merged
test: add cmdtest package #15251
+602
−13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
And add first example tests to the version package. This is a pattern I've used repeatedly and successfully in the past. It offers a simple way to validate that a command emits expected output to stdout and stderr, and the command's returned error. I saw that there was already some functionality around "MockIO" in the testutil package, but that sits at an awkward position where you still have to manage your cobra.Command separately. Moreover, the BufferReader and BufferWriter interfaces it uses are unnecessary -- virtually every test around the CLI will suffice with holding all output in memory, and there is no reason to complicate stdin substitution with anything more complicated than a simple io.Reader. One piece that neither the previous nor the new approach can detect, is use of (*cobra.Command).Println. This has been the source of previous issues where output was expected to go to stdout but instead was sent to stderr. Per the docs on Println, "Println is a convenience method to Println to the defined output, fallback to Stderr if not set." Our tests do call (*cobra.Command).SetOutput, so application code using cmd.Println will work as intended, but this does not force third parties to call SetOutput correctly. We may be able to set up a PersistentPreRun function that checks if (*cobra.Command).OutOrStderr is in fact os.Stderr, and if so, warn or panic. On the other hand, if we are careful to always print to cmd.OutOrStdout() or cmd.ErrOrStderr() appropriately, then hopefully third party applications would follow suit and more consistently print to the intended stream.
This is an example of a slightly more specialized use of (*cmdtest.System). In this set of tests, we have a type ExportSystem that wraps a cmdtest.System and always passes an associated context in its Run and MustRun methods. The export CLI does not accept any information on stdin, so no RunWithInput methods are exposed.
github-prbot
requested review from
a team and
kocubinski
and removed request for
a team
mark-rushakoff
commented
Mar 2, 2023
| @@ -78,5 +78,14 @@ type ( | |||
|
|
|||
| // AppExporter is a function that dumps all app state to | |||
| // JSON-serializable structure and returns the current validator set. | |||
| AppExporter func(log.Logger, dbm.DB, io.Writer, int64, bool, []string, AppOptions, []string) (ExportedApp, error) | |||
| AppExporter func( | |||
| logger log.Logger, | |||
There was a problem hiding this comment.
I only added names for the arguments in this call, to make it slightly easier to understand what an implementation of AppExporter should expect.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if err != nil { | ||
| return err | ||
| } | ||
| defer f.Close() |
Check failure
Code scanning / gosec
Deferring unsafe method "Close" on type "io.WriteCloser"
This is an exceptional case where ctx doesn't belong as the first argument in a method.
julienrbrt
reviewed
Mar 2, 2023
| ) | ||
|
|
||
| // System is a system under test. | ||
| type System struct { |
There was a problem hiding this comment.
This is cool; however, I find the term System a bit ambiguous.
Maybe it's my perception, but if I think of system the two first things that come in my mind are operating system and file system.
I don't have a better term 😅, but it is not immediately clear that it is for testing cobra commands.
|
I agree it’s ambiguous as a type name. I’ll give it some more thought, and
I’m open to suggestions, of course.
…On Thu, Mar 2, 2023 at 6:34 PM Julien Robert ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In testutil/cmdtest/system.go
<#15251 (comment)>:
> @@ -0,0 +1,120 @@
+// Package cmdtest contains a framework for testing cobra Commands within Go unit tests.
+package cmdtest
+
+import (
+ "bytes"
+ "context"
+ "io"
+
+ "github.com/spf13/cobra"
+)
+
+// System is a system under test.
+type System struct {
This is cool; however, I find the term System a bit ambiguous.
I don't have a better term 😅, but it is not immediately clear that it is
for testing cobra commands.
—
Reply to this email directly, view it on GitHub
<#15251 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACG6V4TDPKXAEOQOMIPGD3W2EU7ZANCNFSM6AAAAAAVNXWY4I>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
tac0turtle
reviewed
Mar 6, 2023
alexanderbez
approved these changes
Mar 6, 2023
julienrbrt
approved these changes
Mar 6, 2023
| @@ -0,0 +1,120 @@ | |||
| // Package cmdtest contains a framework for testing cobra Commands within Go unit tests. | |||
| package cmdtest | |||
There was a problem hiding this comment.
nit, maybe this should be under testutil/cli like the other cli testutils.
mark-rushakoff
added
the
A:automerge
19 tasks
12 tasks
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces the
cmdtestpackage, offering a lightweight wrapper around cobra commands to simplify testing CLI utilities.I backfilled tests for the
versioncommand, which was an example of a very simple test setup; and for theexportcommand, which was more involved due to the server and client context requirements.I did notice that there are some existing tests for some utilities, but the
cmdtestpackage follows a simple pattern that has been easy to use successfully in the relayer and in other projects outside the Cosmos ecosystem.While filling in these tests, I started removing uses of
cmd.Print, as that is the root cause of issues like #8498, #7964, #15167, and possibly others. Internal to cobra, the print family of methods write tocmd.OutOrStderr()-- meaning that if the authors callcmd.SetOutput()before executing the command, the output will be written to stdout as expected; otherwise it will go to stderr. I don't understand why that would be the default behavior, but it is probably too late to change from cobra's side.Instead of
cmd.Print, we prefer tofmt.Fprint(cmd.OutOrStdout())orfmt.Fprint(cmd.ErrOrStderr())as appropriate, giving an unambiguous destination for output. And the new tests collect those outputs in plainbytes.Buffervalues so that we can assert their content appropriately.In the longer term, I would like to deprecate and eventually remove the
testutilpackage'sApplyMockIOmethod and itsBufferWriterandBufferReadertypes, as they are unnecessary indirection when a simpler solution exists. But that can wait untilcmdtesthas propagated through the codebase more.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...
provided a link to the relevant issue or specificationReviewers 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...