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.

Sign up for GitHub

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

Jump to bottom

CLI unit-testing for all commands with at least one argument #2352

Merged
merged 32 commits into from
Jul 4, 2022
Merged

CLI unit-testing for all commands with at least one argument #2352

merged 32 commits into from
Jul 4, 2022

Conversation

ljoss17
Copy link
Contributor

@ljoss17 ljoss17 commented Jun 29, 2022 •
edited
Loading

Closes: #2358

Description

This PR adds unit-testing parsing Hermes commands, as well as improving the commands help output.
The help output follows the request from issue #2349. Changes for issue #2349 will be done in a separate PR.

Changes

This PR has the following changes:

  • Adding unit tests for all Hermes commands with at least one argument.
  • Instead of manually validating the flags, the validation is done by clap for commands create channel, create connection, keys add and keys delete.
  • Minor changes to the guide.
  • Updated the ADR 010 with the new --yes optional flag for the command create channel.

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

ljoss17 and others added 22 commits June 7, 2022 11:31
@ljoss17
Updated CLI commands to take flags everywhere and updated e2e tests a…
51852ae 
…ccordingly
@ljoss17
Updated gm to use flags when calling Hermes
21580d3
@ljoss17
Added missing flags to e2e test for 'query client state' command
09c157f
@ljoss17
Fixed flag errors in e2e tests and removed conflicting short flag.
612a707
@ljoss17
Removed all short flags and updated CLI commands comments
88474db
@ljoss17
Removed forgotten short flags.
a76599f
@ljoss17
Updated Hermes guide with flags instead of positional arguments
bf60ca1
@ljoss17
Updated script and comment with new long flags for Hermes
b9983ef
@ljoss17
Completed 'tx raw upgrade-' commands guide page. Updated Testing clie…
eb13cd0 
…nt upgrade guide page
@ljoss17
Added changelog entry
7a25573
@ljoss17
Added example unit-tests to the 'keys add' command
2e3fff3
@ljoss17
Added value names to parameters and removed cli parsing unit-tests
ff84119
@ljoss17
Cargo fmt changes
4d35800
@ljoss17
Updated flags in order to reflect ADR 010
c7e1b9a
@ljoss17
Updated guide to reflect flag changes from ADR 010
c363e04
@ljoss17
Updated gm script and e2e tests to match flag changes from ADR 010
9e6d309
@ljoss17
Merge branch 'master' into luca_joss/replace_positional_args_with_flags
52f5542
@ljoss17
Fixed ADR 010 typo
89bfbf6
@ljoss17
Added unit-testing CLI argument parsing
433621d
@ljoss17
Merge branch 'master' into luca_joss/hermes_cli_unit_tests
e56b90e
@ljoss17
Updated unit tests for CLI and improved help output
26a46c9
@ljoss17
Improved 'override_usage' for commands with multiple usages
ddd9913
@ljoss17
Copy link
Contributor Author

ljoss17 commented Jun 29, 2022 •
edited
Loading

  • Create a new channel between two chains using a pre-existing connection is wrong, that is not the only option. Seems that a blank line (create.rs) in the description drops everything after when displayed with -h
  • the USAGE is wrong. not sure how much control we have with abscissa and clap but --a-chain appears both in USAGE and in OPTIONS. The required flags should not be [OPTIONS]
    • Not sure about this in fact as we wouldn't have a way to describe the required flags. Maybe have a [REQUIRED] section
  • the list of options in alphabetical order makes reading them hard, e.g. -h, --help appears in the middle of the option list. Is there a way to control the order in which they are listed?
  • completion seems to work for the first few options and then it stops working.

@ancazamfir regarding the issues you found, I was able to fix the following:

  • Changing the short message for the short help, -h, of create channel and improving the message for the long help, --help.
  • Changed the USAGE for create channel, create connection, keys add and keys delete to include the two different usages.
  • Added a FLAGS header for required flags, in addition to the OPTIONS header for optional flags, for all the commands.

What do you think about these fixes for the first three issues ?

Regarding the ordering, it is possible to arbitrarily reorder the list of options/flags, but I am not sure which order would be better than alphabetical order. Do you have a preference for this ?

I will try to reproduce the completion issue and if I manage to, try and fix it.

EDIT: These changes will be done in a separate PR.

@romac romac self-assigned this Jun 29, 2022
@ljoss17 ljoss17 changed the title CLI unit-testing and improved help messages CLI unit-testing for all commands with at least one argument Jun 30, 2022
@ljoss17 ljoss17 marked this pull request as ready for review June 30, 2022 10:11
romac
romac approved these changes Jul 1, 2022
View reviewed changes
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

❤️ these unit tests! Great job, @ljoss17 🎉

romac
romac reviewed Jul 1, 2022
View reviewed changes
relayer-cli/src/commands/create/channel.rs
value_name = "A_CONNECTION_ID",
help = "Identifier of the connection on chain `a` to use in creating the new channel."
required = true,
groups = &["b_chain_group", "new_client_group"],
Copy link
Member

Choose a reason for hiding this comment

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

Sweet :)

@romac romac merged commit 058cbe9 into master Jul 4, 2022
@romac romac deleted the luca_joss/hermes_cli_unit_tests branch July 4, 2022 12:39
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@ljoss17 @romac
CLI unit-testing for all commands with at least one argument (informa…
1a40837 
…lsystems#2352)

* Adding unit tests for all Hermes commands with at least one argument.
* Instead of manually validating the flags, the validation is done by `clap` for commands `create channel`, `create connection`, `keys add` and `keys delete`.
* Minor changes to the guide.
* Updated the ADR 010 with the new `--yes` optional flag for the command `create channel`.

---

* Updated CLI commands to take flags everywhere and updated e2e tests accordingly

* Updated gm to use flags when calling Hermes

* Added missing flags to e2e test for 'query client state' command

* Fixed flag errors in e2e tests and removed conflicting short flag.

* Removed all short flags and updated CLI commands comments

* Removed forgotten short flags.

* Updated Hermes guide with flags instead of positional arguments

* Updated script and comment with new long flags for Hermes

* Completed 'tx raw upgrade-' commands guide page. Updated Testing client upgrade guide page

* Added changelog entry

* Added example unit-tests to the 'keys add' command

* Added value names to parameters and removed cli parsing unit-tests

* Cargo fmt changes

* Updated flags in order to reflect ADR 010

* Updated guide to reflect flag changes from ADR 010

* Updated gm script and e2e tests to match flag changes from ADR 010

* Fixed ADR 010 typo

* Added unit-testing CLI argument parsing

* Updated unit tests for CLI and improved help output

* Improved 'override_usage' for commands with multiple usages

* Updated Hermes guide. Removed changes for issue informalsystems#2349, they will be added in a separate PR.

* Removed manual validation for 'keys add' and 'keys delete' flags.

* Added changelog entry

* Updated unit tests for command 'clear packets' to include new optional flags

* Cargo fmt

* Fixed 'keys add' in ADR 010 and added shell completion unit test for case with no flag

* Fix clippy warnings introduced in Rust 1.62

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romac romac romac approved these changes

@mzabaluev mzabaluev Awaiting requested review from mzabaluev

@ancazamfir ancazamfir Awaiting requested review from ancazamfir

@seanchen1991 seanchen1991 Awaiting requested review from seanchen1991

@adizere adizere Awaiting requested review from adizere

Assignees

@romac romac

@mzabaluev mzabaluev

@seanchen1991 seanchen1991

@plafer plafer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding unit tests for Hermes commands parsing
5 participants
@ljoss17 @romac @mzabaluev @seanchen1991 @plafer