search

assert-rs / snapbox

Snapshot testing for a herd of CLI tests
docs.rs/trycmd
Apache License 2.0
137 stars 18 forks source link

trycmd should normalise some env vars including COLUMNS #361

Open ijackson opened 2 months ago

ijackson commented 2 months ago

Steps

git clone https://gitlab.torproject.org/tpo/core/arti.git
cd arti
git checkout  5e2d5532d002c9df9a4cf6d83cdbe7d6125bc4be
COLUMNS=100 cargo test --locked --offline -p arti --all-features -- cli_tests
COLUMNS=1000 cargo test --locked --offline -p arti --all-features -- cli_tests

Expected output

  1. Successful run
  2. Successful run

Actual output

  1. Successful run
  2. Report of differences, apparently due to line wrapping

Discussion

Programs using clap, and many similar, may pay attention to COLUMNS.

Probably, the variable should be set rather than left unset. After, all, a conscientious program which finds it has no COLUMNS might fall back on interrogating the tty with termios ioctls.

There may be other relevant env vars. Maybe grepping the source code to clap would be helpful. It would also be nice if the API had a way to pass additional env vars as a workaround.

(edited to correct the wrong git commitid)

epage commented 2 months ago

I lean towards giving users control over this rather than setting policy for how programs are tested.

gabi-250 commented 2 months ago

It would also be nice if the API had a way to pass additional env vars as a workaround.

We should be able to use TestCases::env here, but I agree it would be better if trycmd set COLUMNS to a reasonable default (if unset).

ijackson commented 2 months ago

I lean towards giving users control over this rather than setting policy for how programs are tested.

I agree with the idea of giving users control. I don't think that control is most conveniently achieved by honouring whatever COLUMNS is in the outer environment.

We should be able to use TestCases::env here

Oh yes, sorry, I had missed that. So I agree with @gabi-250's suggestion.

epage commented 2 months ago

Ah, I hadn't looked to see if we already provided something.

While Cargo doesn't use trycmd, I think looking at its env variables it sets on tests is useful https://github.com/rust-lang/cargo/blob/ef854d2f66df7bfcd803a6ca1cfa3b619603142c/crates/cargo-test-support/src/lib.rs#L1388-L1478

That is a sliding scale from "this is cargo specific" to "this is like COLUMNS" which includes what domains the application touches (e.g. git). At this time, I don't feel up to arbitrating which variables we process automatically for users and which we don't.