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

--color=always doesn't flow through to diffs #367

Closed
max-sixty opened this issue Apr 10, 2023 · 5 comments
Closed

--color=always doesn't flow through to diffs #367

max-sixty opened this issue Apr 10, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@max-sixty
Copy link
Sponsor Contributor

What happened?

Currently --color=always doesn't affect the colorization of diffs

Reproduction steps

Take a failing test:

cargo insta test --color=always -p prql-compiler --lib --check -- test_upper

gives color in the diffs:

image

But when piped to a file, it only has color in the cargo parts, not the diffs:

�[0m�[0m�[1m�[32m    Finished�[0m test [unoptimized + debuginfo] target(s) in 0.63s
�[0m�[0m�[1m�[32m     Running�[0m unittests src/lib.rs (target/debug/deps/prql_compiler-454bbecdaff4c0a9)

running 1 test
test tests::test::test_upper ... FAILED

failures:

---- tests::test::test_upper stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot: upper
Source: prql-compiler/src/tests/test.rs:3436
────────────────────────────────────────────────────────────────────────────────
Expression: compile(
    r#"
    from test_tbles
    derive [upper_name = upper name]
    select [upper_name]
    "#,
)
.unwrap()
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
    0     0 │ SELECT␊
    1     1 │   UPPER(name) AS upper_name␊
    2     2 │ FROM␊
    3       │-  test_tables
          3 │+  test_tbles␊
────────────┴───────────────────────────────────────────────────────────────────
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
thread 'tests::test::test_upper' panicked at 'snapshot assertion for 'upper' failed in line 3436', /Users/maximilian/.cargo/registry/src/github.com-1ecc6299db9ec823/insta-1.29.0/src/runtime.rs:569:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::test::test_upper

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 148 filtered out; finished in 0.10s

�[0m�[0m�[1m�[31merror�[0m�[1m:�[0m test failed, to rerun pass `-p prql-compiler --lib`
�[0m�[0m�[1m�[31merror�[0m�[1m:�[0m 1 target failed:
    `-p prql-compiler --lib`

This also occurs for CARGO_TERM_COLOR

Insta Version

cargo-insta 1.29.0

rustc Version

1.65

What did you expect?

As above

@max-sixty max-sixty added the bug Something isn't working label Apr 10, 2023
@mitsuhiko
Copy link
Owner

Very surprised about this. Both insta (which renders the diff) and cargo-insta use console so they should share that setting.

@xxchan
Copy link

xxchan commented May 15, 2023

Actually this part by cargo test runner is also not colored.

running 1 test
test tests::test::test_upper ... FAILED

But by cargo test --color=always -- --color=always, according to (rust-lang/cargo#1983), you can have that part colored, but the diff is still not colored. 😄

@max-sixty
Copy link
Sponsor Contributor Author

max-sixty commented May 22, 2023

If an example is helpful:

image image

@max-sixty
Copy link
Sponsor Contributor Author

I still don't have a great mental model of this — but at least some of the reason is that libtest uses a different option from cargo, as @xxchan points out:

But by cargo test --color=always -- --color=always, according to (rust-lang/cargo#1983)

...hence the --color=always -- --color=always.

The good news is that the outputs might become consolidated under cargo (rust-lang/cargo#1983 (comment)), which might solve then solve this, or at least make it more tractable

max-sixty added a commit to max-sixty/insta that referenced this issue Jun 2, 2023
Two parts to mitsuhiko#367 :
- The libtest output, which this PR solves
  - I've hand-tested it
  - Possibly this becomes moot if `cargo` is changed to handle more of the output
- The `insta` output when insta is called by libtest; e.g. the diffs. There's not much we can do here, but [`CLICOLOR_FORCE`](http://bixense.com/clicolors/) works
mitsuhiko pushed a commit that referenced this issue Jun 3, 2023
* Pass `--color=...` to libtest

Two parts to #367 :
- The libtest output, which this PR solves
  - I've hand-tested it
  - Possibly this becomes moot if `cargo` is changed to handle more of the output
- The `insta` output when insta is called by libtest; e.g. the diffs. There's not much we can do here, but [`CLICOLOR_FORCE`](http://bixense.com/clicolors/) works

* Update cargo-insta/src/cli.rs
@max-sixty
Copy link
Sponsor Contributor Author

Closed by #375 (I think). Passing CLICOLOR_FORCE in CI seems to work; it's still not clean given --color= doesn't always work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants