Add debug assert to crates-admin cli #7521
Conversation
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
0xPoe
changed the title
Cargo.toml
Outdated
| hyper-tls = "=0.5.0" | ||
| insta = { version = "=1.34.0", features = ["json", "redactions"] } | ||
| tokio = "=1.34.0" | ||
| trycmd = "0.14.19" |
There was a problem hiding this comment.
would it be possible to use insta for this instead? seems unfortunate to add that many new dev dependencies just for asserting the help output of an internal CLI tool 😅
There was a problem hiding this comment.
would it be possible to use
instafor this instead?
Maybe we can do that. But then we would need to handle how to obtain the binary and get the stdout and stderr outputs ourselves. Because insta is just text output, not an emulation for the terminal. So, I think it will be more difficult.
seems unfortunate to add that many new dev dependencies just for asserting the help output of an internal CLI tool
This would help us avoid breaking anything during the big version upgrade. It's not only for one help output, but it also helps with other subcommands(TODO). These tests ensure that we can easily identify changes to the arguments and subcommands.
There was a problem hiding this comment.
seems unfortunate to add that many new dev dependencies
Can you tell me about your concert? Are you concerned about the compilation time?
There was a problem hiding this comment.
But then we would need to handle how to obtain the binary and get the stdout and stderr outputs ourselves.
does clap have an API to get the help output as a String instead?
This would help us avoid breaking anything during the big version upgrade.
what big version upgrade are we talking about? 😅
are there breaking changes on the horizon that would cause behavior changes without compilation errors?
Are you concerned about the compilation time?
yeah, exactly.
I'm not sure yet whether the tradeoff is worth it. If this was a public application where people expect a stable CLI interface then it's definitely worth it, but since this application is an internal tool I'm not sure we need to care that much about interface stability. One notable exception are the subcommands that are configured as cronjobs at Heroku, but those subcommands are very simple and I would be surprised if a clap upgrade broke those without causing compilation errors.
There was a problem hiding this comment.
what big version upgrade are we talking about? 😅
The major version change. Sorry for my bad English. In Chinese, we call the BIG version upgrade.
yeah, exactly.
It increased the building time by about 10s. Anyway, I will drop it.
There was a problem hiding this comment.
does
claphave an API to get the help output as aStringinstead?
It has a render_help API. It will return a StyledStr. I will try it in the future. (If you think it is worth to give a try 😅)
There was a problem hiding this comment.
it looks like https://docs.rs/clap_builder/4.4.8/clap_builder/builder/struct.StyledStr.html implements Display and then produces a string without ANSI escape codes. seems perfect for using it with insta::assert_display_snapshot!(...) :)
0xPoe
force-pushed
the
rustin-patch-amdin-cli
branch
from
d794158 to
c7b32e3
Compare
0xPoe
changed the title
0xPoe
added
C-internal 🔧
| #[test] | ||
| fn verify_cli() { | ||
| use clap::CommandFactory; | ||
| Command::command().debug_assert(); |
There was a problem hiding this comment.
I was curious what debug_assert() is doing. It looks like it runs all of https://docs.rs/clap_builder/4.4.8/src/clap_builder/builder/debug_asserts.rs.html, but only when debug_assertions is enabled, which should be the case when running the test suite. In other words, debug_assert() apparently enables some additional checks that won't even be caught when running the binary from a release build.
tl;dr LGTM 👍
2 participants
Uh oh!
There was an error while loading. Please reload this page.