Skip to content
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

appd export sometimes writes genesis.json to stderr #7964

Closed
4 tasks
michaelfig opened this issue Nov 17, 2020 · 0 comments · Fixed by #8228
Closed
4 tasks

appd export sometimes writes genesis.json to stderr #7964

michaelfig opened this issue Nov 17, 2020 · 0 comments · Fixed by #8228
Labels
Milestone

Comments

@michaelfig
Copy link
Contributor

Summary of Bug

Running appd export writes the new genesis.json to stderr if the appExporter != nil. This violates basic CLI design, mixing the expected output with any error messages.

Version

a680f461c38e4daaf259d6e55473528f168e64cf

Steps to Reproduce

  1. appd export > newgenesis.json shows the genesis.json on stderr
  2. cat newgenesis.json is empty

The following patch makes the newgenesis.json properly output to stdout, as is done for the appExporter == nil case:

diff --git a/server/export.go b/server/export.go
index 150add98e..98eebbf55 100644
--- a/server/export.go
+++ b/server/export.go
@@ -105,7 +105,7 @@ func ExportCmd(appExporter types.AppExporter, defaultNodeHome string) *cobra.Com
                                return err
                        }
 
-                       cmd.Println(string(sdk.MustSortJSON(encoded)))
+                       fmt.Println(string(sdk.MustSortJSON(encoded)))
                        return nil
                },
        }

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@clevinson clevinson added this to the v0.40.1 milestone Nov 23, 2020
@likhita809 likhita809 mentioned this issue Dec 23, 2020
9 tasks
@mergify mergify bot closed this as completed in #8228 Jan 4, 2021
@aaronc aaronc removed the backlog label Jan 4, 2021
mergify bot pushed a commit that referenced this issue Mar 6, 2023
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants