test: Replace trycmd with snapbox#4536
Conversation
epage
force-pushed
the
trycmd
branch
2 times, most recently
from
99e1f74 to
d47278e
Compare
rami3l
left a comment
rami3l
left a comment
There was a problem hiding this comment.
This looks fine to me; however we have to recognize the platform differences as well... Do you happen to have a way to overcome the regarding the failed snapshots on Windows? (Some commands are expected to have different output on Windows than on Unix.)
|
Sorry, had missed that there were failures. Hopefully fixed now. |
|
How do you look at the SVG files locally? What does a test failure look like in a terminal? |
@djc SVGs are just plain text files so you'll be seeing text file diffs. It's not as good as before in terminals but most of the times we are updating the snapshots anyway. I know from @weihanglo that this has been used in |
I use
A diff of XML. If this is limited in its use, then its not bad. We've not had a problem with it in Cargo. Unsure how they are handling it with Rustc. |
|
FWIW: rust-lang/cargo#13461 (comment)
|
|
I definitely review on mobile sometimes. But, let's try it. |
@weihanglo Nice remark. Hmmm can we somehow contact the client support for this? |
epage
force-pushed
the
trycmd
branch
4 times, most recently
from
25cd2d5 to
22d5be9
Compare
| #[test] | ||
| #[cfg(not(windows))] // On windows, we don't have the `man` command | ||
| fn rustup_help_cmd() { | ||
| test_help("rustup_help_cmd", &["help"]); | ||
| } | ||
|
|
||
| #[test] | ||
| #[cfg(not(windows))] // On windows, we don't have the `man` command | ||
| fn rustup_help_flag() { | ||
| test_help("rustup_help_flag", &["--help"]); | ||
| } | ||
|
|
||
| #[test] | ||
| #[cfg(not(windows))] // On windows, we don't have the `man` command | ||
| fn rustup_only_options() { | ||
| test_error("rustup_only_options", &["-q"]); | ||
| } |
There was a problem hiding this comment.
This looks fine to me; however we have to recognize the platform differences as well... Do you happen to have a way to overcome the regarding the failed snapshots on Windows? (Some commands are expected to have different output on Windows than on Unix.)
There was a problem hiding this comment.
So using ... doesn't work with SVG files because it throws off the y variable.
For now, I'm just excluding those tests from Windows.
Alternatives
- We keep only these tests as txt instead of svg
There was a problem hiding this comment.
@epage I'm okay with that given that it seems like the best we could do for now.
OTOH what would the recommended approach be if I am using Windows and just want to draft a PR to make the tests pass?
Is there any prior art in that regard? Otherwise I'm thinking about something like uploading the updated snapshots to the CI results for the developer to download...
I can address that in a separate PR, of course.
There was a problem hiding this comment.
For that scenario, keeping theese as text instead of SVG would be an improvement. On the other hand, it doesn't feel like we get a lot of external contributors that use Windows, so maybe this is rare enough that it's fine? For those cases, it should also be easy enough for non-Windows team members to contribute an updated SVG.
There was a problem hiding this comment.
Okay, let's merge it first and see how it goes :)
There was a problem hiding this comment.
For Windows developers, you can grab the diff from CI. We have to do this in Cargo for platform-specific tests already.
|
CI is now passing though unsure how people feel about the approach I took in #4536 (comment) |
| #[test] | ||
| #[cfg(not(windows))] // On windows, we don't have the `man` command | ||
| fn rustup_help_cmd() { | ||
| test_help("rustup_help_cmd", &["help"]); | ||
| } | ||
|
|
||
| #[test] | ||
| #[cfg(not(windows))] // On windows, we don't have the `man` command | ||
| fn rustup_help_flag() { | ||
| test_help("rustup_help_flag", &["--help"]); | ||
| } | ||
|
|
||
| #[test] | ||
| #[cfg(not(windows))] // On windows, we don't have the `man` command | ||
| fn rustup_only_options() { | ||
| test_error("rustup_only_options", &["-q"]); | ||
| } |
|
Also, it looks like this solves #4415 so that's pretty nice. |
4 participants
This is to prepare for adding styling to help and maybe changing the existing styling (#266). Maybe this will help with #4415 as well.
The layout I went with is:
tests/suite/cli-ui/rustup-init/*.toml->tests/suite/cli_rustup_init_ui.rs+tests/suite/cli_rustup_init_ui/*.term.svgtests/suite/cli-ui/rustup/*.toml->tests/suite/cli_rustup_ui.rs+tests/suite/cli_rustup_ui/*.term.svg