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

Move stdin/stdout/stderr access functionality to cli.context #3558

Closed
jleni opened this issue Feb 8, 2019 · 8 comments
Closed

Move stdin/stdout/stderr access functionality to cli.context #3558

jleni opened this issue Feb 8, 2019 · 8 comments

Comments

@jleni
Copy link
Member

jleni commented Feb 8, 2019

The problem with context is that moving things there results in cyclic dependencies with keys. I will need further refactoring to make context independant from other packages.

The original location of func BufferStdin() *bufio.Reader and in order to inject a different stdin I placed it as near as possible.

I would suggest we create another PR to make context a first-class citizen :) Including stdout too.

Originally posted by @jleni in #3517

@jleni jleni self-assigned this Feb 8, 2019
@jleni
Copy link
Member Author

jleni commented Feb 8, 2019

#3517 (comment)

Include stderr too.

@alessio alessio changed the title Move stdin/stdout access functionality to cli.context Move stdin/stdout/stderr access functionality to cli.context Feb 9, 2019
@jleni
Copy link
Member Author

jleni commented Feb 11, 2019

Cobra already provides this functionality but there are a few small adjustments to make.
I have submitted a PR (spf13/cobra#822) so we can improve testability on our side.

Also I have a proof of concept for Test_RunMnemonicCmdUser that uses this.

We can set mocks for stdin, stdout, stderr at the command level:
https://github.com/ZondaX/cosmos-sdk/blob/0b62457424559580f435d70326f8c0f62cd25dfa/client/keys/mnemonic_test.go#L31-L33

mock inputs:
https://github.com/ZondaX/cosmos-sdk/blob/0b62457424559580f435d70326f8c0f62cd25dfa/client/keys/mnemonic_test.go#L45

and later confirm outputs:
https://github.com/ZondaX/cosmos-sdk/blob/0b62457424559580f435d70326f8c0f62cd25dfa/client/keys/mnemonic_test.go#L61-L64

I/O redirection is at the command level so parallel tests are also possible now.

I would suggest that we use a cobra fork until the PR gets accepted and adjust tests.

@jackzampolin
Copy link
Member

I would really prefer not to run off of a fork, but this really does add quite a bit of flexibility here and will allow for much better CLI testing. Does anyone else have thoughts here @alessio @alexanderbez @fedekunze

@jleni
Copy link
Member Author

jleni commented Feb 11, 2019

I can send a PR for review.. it is not complete but gives an idea of how much better the tests look like

@jackzampolin
Copy link
Member

@jleni Would love to see that!

@jleni
Copy link
Member Author

jleni commented Feb 12, 2019

I have submitted a PR to cobra so this change is accepted upstream and we avoid forking.
spf13/cobra#822

Unfortunately, things are very coupled at the moment.
Tendermint forces spf13/cobra and there are objects being passed between client and server.

CLI tests are a lot better but unless the PR is accepted, it will require Tendermint to use the fork or to decouple by using interfaces.

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jul 17, 2020
@tac0turtle
Copy link
Member

this will be part of the cli v2 work.

@tac0turtle tac0turtle mentioned this issue May 9, 2022
33 tasks
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

No branches or pull requests

3 participants