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

tool: capture stdout / stderr #1998

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Conversation

nicktrav
Copy link
Contributor

@nicktrav nicktrav commented Oct 6, 2022

Currently, the tools write directly to os.{Stdout,Stderr}, which complicates testing in the case where the command is run from same process running the tests (i.e. from a testing.T func).

Adapt the tools to make use of the return values from (*cobra.Command).{OutOrStdout,OutOrStderr}. In testing scenarios, the tools can use SetOut and SetErr to pass in a writer that can intercept anything written to stdout / stderr. In productions scenarios, the tools will emit to the appropriate channel.

Touches cockroachdb/cockroach#89095.

Currently, the tools write directly to `os.{Stdout,Stderr}`, which
complicates testing in the case where the command is run from same
process running the tests (i.e. from a `testing.T` func).

Adapt the tools to make use of the return values from
`(*cobra.Command).{OutOrStdout,OutOrStderr}`. In testing scenarios, the
tools can use `SetOut` and `SetErr` to pass in a writer that can
intercept anything written to stdout / stderr. In productions scenarios,
the tools will emit to the appropriate channel.

Touches cockroachdb/cockroach#89095.
@nicktrav nicktrav requested review from jbowens and a team October 6, 2022 18:47
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@coolcom200 coolcom200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jbowens and @nicktrav)

a discussion (no related file):
LGTM but will defer approval to someone a bit more familiar with this code. I was wondering if there is a way to write a test to ensure that future tools / cobra commands that we write will use the (*cobra.Command).{OutOrStdout,OutOrStderr} for output.


Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nicktrav)

@nicktrav
Copy link
Contributor Author

I was wondering if there is a way to write a test to ensure that future tools / cobra commands that we write will use the (*cobra.Command).{OutOrStdout,OutOrStderr} for output.

Good suggestion. Might be a little tricky. It's only really required for testing, and not correctness.

TFTRs!

@nicktrav nicktrav merged commit f122ff4 into cockroachdb:master Oct 10, 2022
@nicktrav nicktrav deleted the nickt.tool branch October 10, 2022 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants