test(cli): Verify terminal styling #13461

epage · 2024-02-19T20:43:56Z

What does this PR try to resolve?

This uses a new feature from snapbox that let's us render terminal styling in SVG files. This let's us see / visualize ANSI escape codes, including in github's UI (will render images, including side-by-side images for diffs).

How should we test and review this PR?

Potential concerns

term.svg files require two SNAPSHOTS=overwrite runs to be rendered properly assert-rs/snapbox#257: If the snapshot file doesn't exist, you need to SNAPSHOTS=overwrite twice
Cargo doesn't respect --color or CARGO_TERM_COLOR for help text and errors from clap #9012: because we narrow the enabling of colors to cargo via CARGO_TERM_COLOR, clap doesn't know about it and those are rendered without color
File size: du -ch tests reported 13MB before and after
Loss of information: this doesn't yet include links but we already weren't snapshotting these
Unconditionally turning on styling and snapshotting for it for all "ui" tests
Too many SVGs in one PR can negatively affect github's UI

Additional information

rustbot · 2024-02-19T20:44:00Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

weihanglo

This is wild 🤯, but pretty neat!

Do you have an example of side-by-side image diff for this new feature?

epage · 2024-02-19T21:06:38Z

Do you have an example of side-by-side image diff for this new feature?

See rust-cli/anstyle@d5111b0

weihanglo · 2024-02-19T21:06:43Z

One pretty minor concern is that GitHub Mobile renders SVG as text. (Who else review PRs on mobile? 🤷🏾‍♂️)

epage · 2024-02-19T21:09:18Z

One pretty minor concern is that GitHub Mobile renders SVG as text. (Who else review PRs on mobile? 🤷🏾‍♂️)

How viewable do you feel that is to look at the raw SVG? I tried to keep the overhead down to make comparing them in text form easier (and to keep the file sizes down).

weihanglo · 2024-02-19T21:26:32Z

One pretty minor concern is that GitHub Mobile renders SVG as text. (Who else review PRs on mobile? 🤷🏾‍♂️)

How viewable do you feel that is to look at the raw SVG? I tried to keep the overhead down to make comparing them in text form easier (and to keep the file sizes down).

At the point that I won't review them on mobile. Keeping the size down is good, and I don't think we will go back to text diff too often. Even we do, git word diff should be sufficient to help review.

Specific to this PR, the "Files changed" tab is janky for loading images that I cannot even review them, and some of them stop rendering.

epage · 2024-02-19T21:34:11Z

tests/testsuite/cargo_add/git/stderr.term.svg

@@ -0,0 +1,30 @@
+<svg width="831.6px" height="72px" xmlns="http://www.w3.org/2000/svg">


---- expected: tests/testsuite/cargo_remove/invalid_package/stderr.term.svg ++++ actual: stderr 1 - <svg width="890.4000000000001px" height="36px" xmlns="http://www.w3.org/2000/svg"> 1 + <svg width="882px" height="36px" xmlns="http://www.w3.org/2000/svg">

The presence of variable substitution is throwing off the width calculations

Tracking in assert-rs/snapbox#258

epage · 2024-02-19T21:36:14Z

 ---- cargo_add::target::case stdout ----
thread 'cargo_add::target::case' panicked at tests/testsuite/cargo_add/target/mod.rs:34:10:
tests/testsuite/cargo_add/target/stdout.term.svg (expected):
Failed to read /Users/runner/work/cargo/cargo/tests/testsuite/cargo_add/target/stdout.term.svg: No such file or directory (os error 2)

A very generic test name combined with a very broad .gitignore entry caused me to not add these files.

Unsure if we should rename the test or rename the ignore to /target

Rustin170506 · 2024-02-20T13:12:44Z

👍 wow, I never thought we could test CLI like this 🤯

epage · 2024-02-20T15:46:59Z

We should instead not generate snapshots for empty content

test: Remove empty snapshots Inspired by #13461

bors · 2024-02-20T20:51:47Z

☔ The latest upstream changes (presumably #13465) made this pull request unmergeable. Please resolve the merge conflicts.

This uses a new feature from snapbox that let's us render terminal styling in SVG files. This let's us see / visualize ANSI escape codes, including in github's UI (will render images, including side-by-side images for diffs).

epage · 2024-02-22T15:48:36Z

We need to rename the target tests so they don't run into issues with our gitignore file.

Found this with the test for `-Zhelp` in rust-lang#13461.

fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp' ### What does this PR try to resolve? Similar to #9012, we aren't respecting `CARGO_TERM_COLOR` for `-Zhelp` and other places. This corrects that. ### How should we test and review this PR? #9012 was about initialization order to get the value. Here, the problem is that we don't update `Shell` with `CARGO_TERM_COLOR`. In doing this, I was concerned about keeping track of where it is safe to call `config_configure` without running it twice. To this end, I refactored `main` to make it clear that each call to `config_configure` is in a mutually exclusive path that exists immediately. ### Additional information Found this with the test for `-Zhelp` in #13461.

epage · 2024-02-23T16:49:46Z

@rustbot ready

weihanglo

File size: du -ch tests reported 13MB before and after

This is 8.4 to 8.6MB on my machine, which seems pretty acceptable.

BTW, we should update contributor guide https://doc.crates.io/contrib/tests/writing.html after this change.

weihanglo · 2024-02-23T17:58:36Z

I only reviewed 70 of all files changes, but assuming most of the changes are generated, I am going to check my box. If people disagree on it later, we can always revert.

@bors r+

bors · 2024-02-23T17:58:39Z

📌 Commit b14a70f has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-02-23T17:59:46Z

⌛ Testing commit b14a70f with merge 2b302a5...

bors · 2024-02-23T18:39:50Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 2b302a5 to master...

weihanglo · 2024-02-23T20:29:22Z

tests/testsuite/cargo_add/invalid_manifest/stderr.term.svg

+  <text xml:space="preserve" class="container fg">
+    <tspan x="10px" y="28px"><tspan class="fg-bright-red bold">error</tspan><tspan>: </tspan><tspan class="[..]">invalid string</tspan>
+</tspan>
+    <tspan x="10px" y="46px"><tspan class="[..]">expected `"`, `'`</tspan>


Glob ([..]) is used here because annotate-snippets print platform-specific colors. This is a thing worth keeping in mind or writing down in contributor guide

Update cargo 16 commits in 194a60b2952bd5d12ba15dd2577a97eed7d3c587..8964c8ccff6e420e2a38b8696d178d69fab84d9d 2024-02-21 01:53:45 +0000 to 2024-02-27 19:22:46 +0000 - feat: Add "-Zpublic-dependency" for public-dependency feature. (rust-lang/cargo#13340) - Stabilize global cache data tracking. (rust-lang/cargo#13492) - chore: bump baseline version requirement of sub crates (rust-lang/cargo#13494) - fix(doctest): search native libs in build script outputs (rust-lang/cargo#13490) - chore: fixed a typo(two->too) (rust-lang/cargo#13489) - test: relax help text assertion (rust-lang/cargo#13488) - refactor: clean up for `GlobalContext` rename (rust-lang/cargo#13486) - test(cli): Verify terminal styling (rust-lang/cargo#13461) - fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp' (rust-lang/cargo#13479) - Error messages when collecting workspace members now mention the workspace root location (rust-lang/cargo#13480) - fix(add): Improve error when adding registry packages while vendored (rust-lang/cargo#13281) - [docs]:Add missing jump links (rust-lang/cargo#13478) - Add global_cache_tracker stability tests. (rust-lang/cargo#13467) - fix(cli): Control clap colors through config (rust-lang/cargo#13463) - chore: remove the unused function (rust-lang/cargo#13472) - Fix missing brackets (rust-lang/cargo#13470)

Found this with the test for `-Zhelp` in rust-lang#13461.

Update cargo 16 commits in 194a60b2952bd5d12ba15dd2577a97eed7d3c587..8964c8ccff6e420e2a38b8696d178d69fab84d9d 2024-02-21 01:53:45 +0000 to 2024-02-27 19:22:46 +0000 - feat: Add "-Zpublic-dependency" for public-dependency feature. (rust-lang/cargo#13340) - Stabilize global cache data tracking. (rust-lang/cargo#13492) - chore: bump baseline version requirement of sub crates (rust-lang/cargo#13494) - fix(doctest): search native libs in build script outputs (rust-lang/cargo#13490) - chore: fixed a typo(two->too) (rust-lang/cargo#13489) - test: relax help text assertion (rust-lang/cargo#13488) - refactor: clean up for `GlobalContext` rename (rust-lang/cargo#13486) - test(cli): Verify terminal styling (rust-lang/cargo#13461) - fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp' (rust-lang/cargo#13479) - Error messages when collecting workspace members now mention the workspace root location (rust-lang/cargo#13480) - fix(add): Improve error when adding registry packages while vendored (rust-lang/cargo#13281) - [docs]:Add missing jump links (rust-lang/cargo#13478) - Add global_cache_tracker stability tests. (rust-lang/cargo#13467) - fix(cli): Control clap colors through config (rust-lang/cargo#13463) - chore: remove the unused function (rust-lang/cargo#13472) - Fix missing brackets (rust-lang/cargo#13470)

feat: Use consistent colors when testing In cargo#13461, it was [noted that `annotate-snippets`](#13461 (comment)) made testing hard as it outputs platform specific colors. To fix this I created [annotate-snippets-rs#82](rust-lang/annotate-snippets-rs#82), which added a feature to make the output platform agnostic when enabled. This makes it so glob syntax does not need to be used when matching colored output.

Found this with the test for `-Zhelp` in rust-lang#13461.

rustbot assigned weihanglo Feb 19, 2024

rustbot added A-testing-cargo-itself Area: cargo's tests S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2024

weihanglo reviewed Feb 19, 2024

View reviewed changes

epage commented Feb 19, 2024

View reviewed changes

epage force-pushed the snapbox branch from 0fcb8d8 to 174e03b Compare February 19, 2024 21:35

This was referenced Feb 19, 2024

term.svg files get the wrong width calculated when using variable substitution assert-rs/snapbox#258

Closed

fix(svg): Clean up from seeing in use in cargo rust-cli/anstyle#176

Merged

epage mentioned this pull request Feb 20, 2024

test: Remove empty snapshots #13465

Merged

bors added a commit that referenced this pull request Feb 20, 2024

Auto merge of #13465 - epage:delete, r=weihanglo

64ccff2

test: Remove empty snapshots Inspired by #13461

test(cli): Verify terminal styling

b14a70f

This uses a new feature from snapbox that let's us render terminal styling in SVG files. This let's us see / visualize ANSI escape codes, including in github's UI (will render images, including side-by-side images for diffs).

epage force-pushed the snapbox branch 2 times, most recently from 7a73315 to 216a22a Compare February 22, 2024 03:06

weihanglo mentioned this pull request Feb 22, 2024

fix(cli): Control clap colors through config #13463

Merged

epage force-pushed the snapbox branch from 216a22a to 731fa91 Compare February 22, 2024 15:47

epage added a commit to epage/cargo that referenced this pull request Feb 22, 2024

fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp'

4830a4f

Found this with the test for `-Zhelp` in rust-lang#13461.

epage mentioned this pull request Feb 22, 2024

fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp' #13479

Merged

epage force-pushed the snapbox branch 3 times, most recently from f80b8cf to eafda07 Compare February 22, 2024 16:42

epage added a commit to epage/cargo that referenced this pull request Feb 22, 2024

fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp'

2ec0461

Found this with the test for `-Zhelp` in rust-lang#13461.

epage force-pushed the snapbox branch 2 times, most recently from a2c117e to 306d69b Compare February 22, 2024 17:25

epage force-pushed the snapbox branch from 306d69b to b14a70f Compare February 23, 2024 14:46

weihanglo approved these changes Feb 23, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2024

bors merged commit 2b302a5 into rust-lang:master Feb 23, 2024
23 checks passed

This was referenced Feb 23, 2024

Fix: cargo vendor can't handle duplicates. #13271

Open

feat: Add "-Zpublic-dependency" for public-dependency feature. #13340

Merged

weihanglo reviewed Feb 23, 2024

View reviewed changes

weihanglo mentioned this pull request Feb 27, 2024

Update cargo rust-lang/rust#121699

Merged

stupendoussuperpowers pushed a commit to stupendoussuperpowers/cargo that referenced this pull request Feb 28, 2024

fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp'

a9b82ca

Found this with the test for `-Zhelp` in rust-lang#13461.

Muscraft mentioned this pull request Feb 29, 2024

feat: Add feature for testing colored output easier rust-lang/annotate-snippets-rs#82

Merged

estebank mentioned this pull request Mar 1, 2024

On tests that specify --color=always emit SVG file with stderr output rust-lang/rust#121877

Merged

ehuss added this to the 1.78.0 milestone Mar 2, 2024

Muscraft mentioned this pull request Mar 2, 2024

feat: Use consistent colors when testing #13520

Merged

charmitro pushed a commit to charmitro/cargo that referenced this pull request Sep 13, 2024

fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp'

8dc0f04

Found this with the test for `-Zhelp` in rust-lang#13461.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(cli): Verify terminal styling #13461

test(cli): Verify terminal styling #13461

epage commented Feb 19, 2024 •

edited

Loading

rustbot commented Feb 19, 2024

weihanglo left a comment

epage commented Feb 19, 2024

weihanglo commented Feb 19, 2024

epage commented Feb 19, 2024

weihanglo commented Feb 19, 2024

epage Feb 19, 2024

epage Feb 19, 2024

epage commented Feb 19, 2024

Rustin170506 commented Feb 20, 2024

epage commented Feb 20, 2024

bors commented Feb 20, 2024

epage commented Feb 22, 2024

epage commented Feb 23, 2024

weihanglo left a comment

weihanglo commented Feb 23, 2024

bors commented Feb 23, 2024

bors commented Feb 23, 2024

bors commented Feb 23, 2024

weihanglo Feb 23, 2024

		@@ -0,0 +1,30 @@
		<svg width="831.6px" height="72px" xmlns="http://www.w3.org/2000/svg">

test(cli): Verify terminal styling #13461

test(cli): Verify terminal styling #13461

Conversation

epage commented Feb 19, 2024 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Feb 19, 2024

weihanglo left a comment

Choose a reason for hiding this comment

epage commented Feb 19, 2024

weihanglo commented Feb 19, 2024

epage commented Feb 19, 2024

weihanglo commented Feb 19, 2024

epage Feb 19, 2024

Choose a reason for hiding this comment

epage Feb 19, 2024

Choose a reason for hiding this comment

epage commented Feb 19, 2024

Rustin170506 commented Feb 20, 2024

epage commented Feb 20, 2024

bors commented Feb 20, 2024

epage commented Feb 22, 2024

epage commented Feb 23, 2024

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Feb 23, 2024

bors commented Feb 23, 2024

bors commented Feb 23, 2024

bors commented Feb 23, 2024

weihanglo Feb 23, 2024

Choose a reason for hiding this comment

epage commented Feb 19, 2024 •

edited

Loading