test: add cmdtest package (#15251)

## Description

This PR introduces the `cmdtest` package, offering a lightweight wrapper around cobra commands to simplify testing CLI utilities.

I backfilled tests for the `version` command, which was an example of a very simple test setup; and for the `export` command, 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 `cmdtest` package follows a simple pattern that has been easy to use successfully in [the relayer](https://github.com/cosmos/relayer/blob/main/internal/relayertest/system.go) 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 to `cmd.OutOrStderr()` -- meaning that if the authors call `cmd.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 to `fmt.Fprint(cmd.OutOrStdout())` or `fmt.Fprint(cmd.ErrOrStderr())` as appropriate, giving an unambiguous destination for output. And the new tests collect those outputs in plain `bytes.Buffer` values so that we can assert their content appropriately.

In the longer term, I would like to deprecate and eventually remove the `testutil` package's `ApplyMockIO` method and its `BufferWriter` and `BufferReader` types, as they are unnecessary indirection when a simpler solution exists. But that can wait until `cmdtest` has 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...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] ~~provided a link to the relevant issue or specification~~
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### 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...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed all author checklist items have been addressed
- [ ] confirmed that this PR does not change production code
This commit is contained in:
Mark Rushakoff 2023-03-06 14:40:24 -05:00 committed by GitHub
parent f151bf627a
commit e7097b1468
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 602 additions and 13 deletions

View File

@ -1,8 +1,10 @@
package server
import (
"bytes"
"encoding/json"
"fmt"
"io"
"os"
"github.com/spf13/cobra"
@ -24,7 +26,8 @@ func ExportCmd(appExporter types.AppExporter, defaultNodeHome string) *cobra.Com
cmd := &cobra.Command{
Use: "export",
Short: "Export state to JSON",
RunE: func(cmd *cobra.Command, args []string) error {
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, _ []string) error {
serverCtx := GetServerContextFromCmd(cmd)
config := serverCtx.Config
@ -41,16 +44,24 @@ func ExportCmd(appExporter types.AppExporter, defaultNodeHome string) *cobra.Com
}
if appExporter == nil {
if _, err := fmt.Fprintln(os.Stderr, "WARNING: App exporter not defined. Returning genesis file."); err != nil {
if _, err := fmt.Fprintln(cmd.ErrOrStderr(), "WARNING: App exporter not defined. Returning genesis file."); err != nil {
return err
}
genesis, err := os.ReadFile(config.GenesisFile())
// Open file in read-only mode so we can copy it to stdout.
// It is possible that the genesis file is large,
// so we don't need to read it all into memory
// before we stream it out.
f, err := os.OpenFile(config.GenesisFile(), os.O_RDONLY, 0)
if err != nil {
return err
}
defer f.Close()
if _, err := io.Copy(cmd.OutOrStdout(), f); err != nil {
return err
}
fmt.Println(string(genesis))
return nil
}
@ -68,7 +79,7 @@ func ExportCmd(appExporter types.AppExporter, defaultNodeHome string) *cobra.Com
exported, err := appExporter(serverCtx.Logger, db, traceWriter, height, forZeroHeight, jailAllowedAddrs, serverCtx.Viper, modulesToExport)
if err != nil {
return fmt.Errorf("error exporting state: %v", err)
return fmt.Errorf("error exporting state: %w", err)
}
appGenesis, err := genutiltypes.AppGenesisFromFile(serverCtx.Config.GenesisFile())
@ -85,12 +96,10 @@ func ExportCmd(appExporter types.AppExporter, defaultNodeHome string) *cobra.Com
return err
}
cmd.SetOut(cmd.OutOrStdout())
cmd.SetErr(cmd.OutOrStderr())
if outputDocument == "" {
cmd.Println(string(out))
return nil
// Copy the entire genesis file to stdout.
_, err := io.Copy(cmd.OutOrStdout(), bytes.NewReader(out))
return err
}
if err = appGenesis.SaveAs(outputDocument); err != nil {

350
server/export_test.go Normal file
View File

@ -0,0 +1,350 @@
package server_test
import (
"context"
"encoding/json"
"fmt"
"io"
"os"
"path/filepath"
"testing"
"time"
"cosmossdk.io/log"
cmtcfg "github.com/cometbft/cometbft/config"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
cmttypes "github.com/cometbft/cometbft/types"
dbm "github.com/cosmos/cosmos-db"
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/server"
"github.com/cosmos/cosmos-sdk/server/types"
"github.com/cosmos/cosmos-sdk/testutil/cmdtest"
"github.com/cosmos/cosmos-sdk/types/module"
genutilcli "github.com/cosmos/cosmos-sdk/x/genutil/client/cli"
genutiltypes "github.com/cosmos/cosmos-sdk/x/genutil/types"
"github.com/rs/zerolog"
"github.com/spf13/viper"
"github.com/stretchr/testify/require"
)
// ExportSystem wraps a (*cmdtest).System
// and sets up appropriate client and server contexts,
// to simplify testing the export CLI.
type ExportSystem struct {
sys *cmdtest.System
Ctx context.Context
Sctx *server.Context
Cctx client.Context
HomeDir string
}
// newExportSystem returns a cmdtest.System with export as a child command,
// and it returns a context.Background with an associated *server.Context value.
func NewExportSystem(t *testing.T, exporter types.AppExporter) *ExportSystem {
t.Helper()
homeDir := t.TempDir()
// Unclear why we have to create the config directory ourselves,
// but tests fail without this.
if err := os.MkdirAll(filepath.Join(homeDir, "config"), 0o700); err != nil {
t.Fatal(err)
}
sys := cmdtest.NewSystem()
sys.AddCommands(
server.ExportCmd(exporter, homeDir),
genutilcli.InitCmd(module.NewBasicManager(), homeDir),
)
tw := zerolog.NewTestWriter(t)
tw.Frame = 5 // Seems to be the magic number to get source location to match logger calls.
sCtx := server.NewContext(
viper.New(),
cmtcfg.DefaultConfig(),
log.NewCustomLogger(zerolog.New(tw)),
)
sCtx.Config.SetRoot(homeDir)
cCtx := (client.Context{}).WithHomeDir(homeDir)
ctx := context.WithValue(context.Background(), server.ServerContextKey, sCtx)
ctx = context.WithValue(ctx, client.ClientContextKey, &cCtx)
return &ExportSystem{
sys: sys,
Ctx: ctx,
Sctx: sCtx,
Cctx: cCtx,
HomeDir: homeDir,
}
}
// Run wraps (*cmdtest.System).RunC, providing e's context.
func (s *ExportSystem) Run(args ...string) cmdtest.RunResult {
return s.sys.RunC(s.Ctx, args...)
}
// MustRun wraps (*cmdtest.System).MustRunC, providing e's context.
func (s *ExportSystem) MustRun(t *testing.T, args ...string) cmdtest.RunResult {
return s.sys.MustRunC(t, s.Ctx, args...)
}
// isZeroExportedApp reports whether all fields of a are unset.
//
// This is for the mockExporter to check if a return value was ever set.
func isZeroExportedApp(a types.ExportedApp) bool {
return a.AppState == nil &&
len(a.Validators) == 0 &&
a.Height == 0 &&
a.ConsensusParams == nil
}
// mockExporter provides an Export method matching server/types.AppExporter,
// and it tracks relevant arguments when that method is called.
type mockExporter struct {
// The values to return from Export().
ExportApp types.ExportedApp
Err error
// Whether Export was called at all.
WasCalled bool
// Called tracks the interesting arguments passed to Export().
Called struct {
Height int64
ForZeroHeight bool
JailAllowedAddrs []string
ModulesToExport []string
}
}
// SetDefaultExportApp sets a valid ExportedApp to be returned
// when e.Export is called.
func (e *mockExporter) SetDefaultExportApp() {
e.ExportApp = types.ExportedApp{
ConsensusParams: &cmtproto.ConsensusParams{
Block: &cmtproto.BlockParams{
MaxBytes: 5 * 1024 * 1024,
MaxGas: -1,
},
Evidence: &cmtproto.EvidenceParams{
MaxAgeNumBlocks: 100,
MaxAgeDuration: time.Hour,
MaxBytes: 1024 * 1024,
},
Validator: &cmtproto.ValidatorParams{
PubKeyTypes: []string{cmttypes.ABCIPubKeyTypeEd25519},
},
},
}
}
// Export satisfies the server/types.AppExporter function type.
//
// e tracks relevant arguments under the e.Called struct.
//
// Export panics if neither e.ExportApp nor e.Err have been set.
func (e *mockExporter) Export(
logger log.Logger,
db dbm.DB,
traceWriter io.Writer,
height int64,
forZeroHeight bool,
jailAllowedAddrs []string,
opts types.AppOptions,
modulesToExport []string,
) (types.ExportedApp, error) {
if e.Err == nil && isZeroExportedApp(e.ExportApp) {
panic(fmt.Errorf("(*mockExporter).Export called without setting e.ExportApp or e.Err"))
}
e.WasCalled = true
e.Called.Height = height
e.Called.ForZeroHeight = forZeroHeight
e.Called.JailAllowedAddrs = jailAllowedAddrs
e.Called.ModulesToExport = modulesToExport
return e.ExportApp, e.Err
}
func TestExportCLI(t *testing.T) {
// Use t.Parallel in all of the subtests,
// because they all read from disk and risk blocking on io.
t.Run("fail on missing genesis file", func(t *testing.T) {
t.Parallel()
e := new(mockExporter)
sys := NewExportSystem(t, e.Export)
res := sys.Run("export")
require.Error(t, res.Err)
require.Truef(t, os.IsNotExist(res.Err), "expected resulting error to be os.IsNotExist, got %T (%v)", res.Err, res.Err)
require.False(t, e.WasCalled)
})
t.Run("prints to stdout by default", func(t *testing.T) {
t.Parallel()
e := new(mockExporter)
e.SetDefaultExportApp()
sys := NewExportSystem(t, e.Export)
_ = sys.MustRun(t, "init", "some_moniker")
res := sys.MustRun(t, "export")
require.Empty(t, res.Stderr.String())
CheckExportedGenesis(t, res.Stdout.Bytes())
})
t.Run("passes expected default values to the AppExporter", func(t *testing.T) {
t.Parallel()
e := new(mockExporter)
e.SetDefaultExportApp()
sys := NewExportSystem(t, e.Export)
_ = sys.MustRun(t, "init", "some_moniker")
_ = sys.MustRun(t, "export")
require.True(t, e.WasCalled)
require.Equal(t, int64(-1), e.Called.Height)
require.False(t, e.Called.ForZeroHeight)
require.Empty(t, e.Called.JailAllowedAddrs)
require.Empty(t, e.Called.ModulesToExport)
})
t.Run("passes flag values to the AppExporter", func(t *testing.T) {
t.Parallel()
e := new(mockExporter)
e.SetDefaultExportApp()
sys := NewExportSystem(t, e.Export)
_ = sys.MustRun(t, "init", "some_moniker")
_ = sys.MustRun(t, "export",
"--height=100",
"--jail-allowed-addrs", "addr1,addr2",
"--modules-to-export", "foo,bar",
)
require.True(t, e.WasCalled)
require.Equal(t, int64(100), e.Called.Height)
require.False(t, e.Called.ForZeroHeight)
require.Equal(t, []string{"addr1", "addr2"}, e.Called.JailAllowedAddrs)
require.Equal(t, []string{"foo", "bar"}, e.Called.ModulesToExport)
})
t.Run("passes --for-zero-height to the AppExporter", func(t *testing.T) {
t.Parallel()
e := new(mockExporter)
e.SetDefaultExportApp()
sys := NewExportSystem(t, e.Export)
_ = sys.MustRun(t, "init", "some_moniker")
_ = sys.MustRun(t, "export", "--for-zero-height")
require.True(t, e.WasCalled)
require.Equal(t, int64(-1), e.Called.Height)
require.True(t, e.Called.ForZeroHeight)
require.Empty(t, e.Called.JailAllowedAddrs)
require.Empty(t, e.Called.ModulesToExport)
})
t.Run("prints to a given file with --output-document", func(t *testing.T) {
t.Parallel()
e := new(mockExporter)
e.SetDefaultExportApp()
sys := NewExportSystem(t, e.Export)
_ = sys.MustRun(t, "init", "some_moniker")
outDir := t.TempDir()
outFile := filepath.Join(outDir, "export.json")
res := sys.MustRun(t, "export", "--output-document", outFile)
require.Empty(t, res.Stderr.String())
require.Empty(t, res.Stdout.String())
j, err := os.ReadFile(outFile)
require.NoError(t, err)
CheckExportedGenesis(t, j)
})
t.Run("prints genesis to stdout when no app exporter defined", func(t *testing.T) {
t.Parallel()
sys := NewExportSystem(t, nil)
_ = sys.MustRun(t, "init", "some_moniker")
res := sys.MustRun(t, "export")
require.Contains(t, res.Stderr.String(), "WARNING: App exporter not defined.")
origGenesis, err := os.ReadFile(filepath.Join(sys.HomeDir, "config", "genesis.json"))
require.NoError(t, err)
out := res.Stdout.Bytes()
require.Equal(t, origGenesis, out)
})
t.Run("returns app exporter error", func(t *testing.T) {
t.Parallel()
e := new(mockExporter)
e.Err = fmt.Errorf("whoopsie")
sys := NewExportSystem(t, e.Export)
_ = sys.MustRun(t, "init", "some_moniker")
res := sys.Run("export")
require.ErrorIs(t, res.Err, e.Err)
})
t.Run("rejects positional arguments", func(t *testing.T) {
t.Parallel()
e := new(mockExporter)
e.SetDefaultExportApp()
sys := NewExportSystem(t, e.Export)
_ = sys.MustRun(t, "init", "some_moniker")
outDir := t.TempDir()
outFile := filepath.Join(outDir, "export.json")
res := sys.Run("export", outFile)
require.Error(t, res.Err)
require.NoFileExists(t, outFile)
})
}
// CheckExportedGenesis fails t if j cannot be unmarshaled into a valid AppGenesis.
func CheckExportedGenesis(t *testing.T, j []byte) {
t.Helper()
var ag genutiltypes.AppGenesis
require.NoError(t, json.Unmarshal(j, &ag))
require.NotEmpty(t, ag.AppName)
require.NotZero(t, ag.GenesisTime)
require.NotEmpty(t, ag.ChainID)
require.NotNil(t, ag.Consensus)
}

View File

@ -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,
db dbm.DB,
traceWriter io.Writer,
height int64,
forZeroHeight bool,
jailAllowedAddrs []string,
opts AppOptions,
modulesToExport []string,
) (ExportedApp, error)
)

120
testutil/cmdtest/system.go Normal file
View File

@ -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 {
commands []*cobra.Command
}
// NewSystem returns a new System.
func NewSystem() *System {
// We aren't doing any special initialization yet,
// but let's encourage a constructor to make it simpler
// to update later, if needed.
return new(System)
}
// AddCommands sets commands to be available to the Run family of methods on s.
func (s *System) AddCommands(cmds ...*cobra.Command) {
s.commands = append(s.commands, cmds...)
}
// RunResult is the stdout and stderr resulting from a call to a System's Run family of methods,
// and any error that was returned.
type RunResult struct {
Stdout, Stderr bytes.Buffer
Err error
}
// Run calls s.RunC with context.Background().
func (s *System) Run(args ...string) RunResult {
return s.RunC(context.Background(), args...)
}
// RunC calls s.RunWithInput with an empty stdin.
func (s *System) RunC(ctx context.Context, args ...string) RunResult {
return s.RunWithInputC(ctx, bytes.NewReader(nil), args...)
}
// RunWithInput calls s.RunWithInputC with context.Background().
func (s *System) RunWithInput(in io.Reader, args ...string) RunResult {
return s.RunWithInputC(context.Background(), in, args...)
}
// RunWithInputC executes a new root command with subcommands
// that were set in s.AddCommands().
// The command's stdin is set to the in argument.
// RunWithInputC returns a RunResult wrapping stdout, stderr, and any returned error.
func (s *System) RunWithInputC(ctx context.Context, in io.Reader, args ...string) RunResult {
rootCmd := &cobra.Command{}
rootCmd.AddCommand(s.commands...)
rootCmd.SetIn(in)
var res RunResult
rootCmd.SetOutput(&res.Stdout)
rootCmd.SetErr(&res.Stderr)
rootCmd.SetArgs(args)
res.Err = rootCmd.ExecuteContext(ctx)
return res
}
// MustRun calls s.Run, but also calls t.FailNow if RunResult.Err is not nil.
func (s *System) MustRun(t TestingT, args ...string) RunResult {
t.Helper()
return s.MustRunC(t, context.Background(), args...)
}
// MustRunC calls s.RunWithInput, but also calls t.FailNow if RunResult.Err is not nil.
func (s *System) MustRunC(t TestingT, ctx context.Context, args ...string) RunResult { //nolint:revive // As a variation of MustRun, t is more important than ctx.
t.Helper()
return s.MustRunWithInputC(t, ctx, bytes.NewReader(nil), args...)
}
// MustRunWithInput calls s.RunWithInput, but also calls t.FailNow if RunResult.Err is not nil.
func (s *System) MustRunWithInput(t TestingT, in io.Reader, args ...string) RunResult {
t.Helper()
return s.MustRunWithInputC(t, context.Background(), in, args...)
}
// MustRunWithInputC calls s.RunWithInputC, but also calls t.FailNow if RunResult.Err is not nil.
func (s *System) MustRunWithInputC(t TestingT, ctx context.Context, in io.Reader, args ...string) RunResult { //nolint:revive // As a variation of MustRun, t is more important than ctx.
t.Helper()
res := s.RunWithInputC(ctx, in, args...)
if res.Err != nil {
t.Logf("Error executing %v: %v", args, res.Err)
t.Logf("Stdout: %q", res.Stdout.String())
t.Logf("Stderr: %q", res.Stderr.String())
t.FailNow()
}
return res
}
// TestingT is a subset of testing.TB,
// containing only what the (*System).Must methods use.
//
// This simplifies using other testing wrappers,
// such as testify suite, etc.
type TestingT interface {
Helper()
Logf(format string, args ...any)
FailNow()
}

View File

@ -2,6 +2,7 @@ package version
import (
"encoding/json"
"fmt"
"strings"
"github.com/cometbft/cometbft/libs/cli"
@ -16,11 +17,12 @@ func NewVersionCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "version",
Short: "Print the application binary version information",
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, _ []string) error {
verInfo := NewInfo()
if long, _ := cmd.Flags().GetBool(flagLong); !long {
cmd.Println(verInfo.Version)
fmt.Fprintln(cmd.OutOrStdout(), verInfo.Version)
return nil
}
@ -42,7 +44,7 @@ func NewVersionCommand() *cobra.Command {
return err
}
cmd.Println(string(bz))
fmt.Fprintln(cmd.OutOrStdout(), string(bz))
return nil
},
}

View File

@ -4,6 +4,7 @@ import (
"encoding/json"
"fmt"
"runtime"
"strings"
"testing"
"github.com/cometbft/cometbft/libs/cli"
@ -11,6 +12,7 @@ import (
"github.com/stretchr/testify/require"
"github.com/cosmos/cosmos-sdk/testutil"
"github.com/cosmos/cosmos-sdk/testutil/cmdtest"
"github.com/cosmos/cosmos-sdk/version"
)
@ -40,6 +42,103 @@ go version go1.14 linux/amd64`
require.Equal(t, want, info.String())
}
func TestCLI(t *testing.T) {
setVersionPackageVars(t)
sys := cmdtest.NewSystem()
sys.AddCommands(version.NewVersionCommand())
t.Run("no flags", func(t *testing.T) {
res := sys.MustRun(t, "version")
// Only prints the version, with a newline, to stdout.
require.Equal(t, testVersion+"\n", res.Stdout.String())
require.Empty(t, res.Stderr.String())
})
t.Run("--long flag", func(t *testing.T) {
res := sys.MustRun(t, "version", "--long")
out := res.Stdout.String()
lines := strings.Split(out, "\n")
require.Contains(t, lines, "name: testchain-app")
require.Contains(t, lines, "server_name: testchaind")
require.Contains(t, lines, `version: "3.14"`)
require.Contains(t, lines, "commit: abc123")
require.Contains(t, lines, "build_tags: mybuildtag")
require.Empty(t, res.Stderr.String())
})
t.Run("--output=json flag", func(t *testing.T) {
res := sys.MustRun(t, "version", "--output=json")
var info version.Info
require.NoError(t, json.Unmarshal(res.Stdout.Bytes(), &info))
// Assert against a couple fields that are difficult to predict in test
// without copying and pasting code.
require.NotEmpty(t, info.GoVersion)
// The SDK version appears to not be set during this test, so we'll ignore it here.
// Now clear out the non-empty fields, so we can compare against a fixed value.
info.GoVersion = ""
want := version.Info{
Name: testName,
AppName: testAppName,
Version: testVersion,
GitCommit: testCommit,
BuildTags: testBuildTags,
}
require.Equal(t, want, info)
require.Empty(t, res.Stderr.String())
})
t.Run("positional args rejected", func(t *testing.T) {
res := sys.Run("version", "foo")
require.Error(t, res.Err)
})
}
const (
testName = "testchain-app"
testAppName = "testchaind"
testVersion = "3.14"
testCommit = "abc123"
testBuildTags = "mybuildtag"
)
// setVersionPackageVars temporarily overrides the package variables in the version package
// so that we can assert meaningful output.
func setVersionPackageVars(t *testing.T) {
t.Helper()
var (
origName = version.Name
origAppName = version.AppName
origVersion = version.Version
origCommit = version.Commit
origBuildTags = version.BuildTags
)
t.Cleanup(func() {
version.Name = origName
version.AppName = origAppName
version.Version = origVersion
version.Commit = origCommit
version.BuildTags = origBuildTags
})
version.Name = testName
version.AppName = testAppName
version.Version = testVersion
version.Commit = testCommit
version.BuildTags = testBuildTags
}
func Test_runVersionCmd(t *testing.T) {
cmd := version.NewVersionCommand()
_, mockOut := testutil.ApplyMockIO(cmd)