oli-obk / ui_test

A test framework for testing rustc diagnostics output
24 stars 24 forks source link

Normalize line endings in `.stderr`/`.stdout` files if there is no `.gitattributes` file #177

Open xFrednet opened 12 months ago

xFrednet commented 12 months ago

Marker uses ui_test in the CI and runs them on Ubuntu, MacOS, and windows. The $DIR value replacement for paths works very well for unix-based systems with / but struggles on windows. The default setup with ui_test::Config::rustc(ui_dir), doesn't seem to replace windows paths.

In the main Marker repo, it was sufficient to add the following filters:

    config.filter(r"\\/", "/");
    config.filter(r"\\\\", "/");

However, the same filters don't seem to work on another lint crate, when ran on windows. This is the error output I get even with the filters:

warning: almost complete ascii range
Warning:   --> tests/ui\almost_complete_range.rs:13:13
   |
13 |     let _ = ('A'..'Z').contains(&c);
   |             ^^^^^^^^^^ help: try: `'A'..='A'`

I assume that this is just a configuration error somewhere on my end. However, it would be nice if the path replacement could be improved to handle windows better.

Replacing all \\ with something else, is also not ideal, as it was pointed out in https://github.com/oli-obk/ui_test/issues/89

oli-obk commented 12 months ago

The order of filters may be important. You can specify the backslash filter again at the end.

Are you overriding the default filters by any chance?

We do have https://github.com/oli-obk/ui_test/blob/5898e04e5b0d5213e223bea93dbb5ac06f7bf391/src/lib.rs#L56, which we could use to cover your situation, too, once we know what the issue is.

Alexendoo commented 12 months ago

It doesn't pick it up because of the mixed slashes in the path

FAILED TEST: tests/ui\almost_complete_range.rs

Rust paths have this incredibly annoying property on windows where they don't correct the slashes, you can hack around that by changing https://docs.rs/marker_uitest/latest/src/marker_uitest/lib.rs.html#41 to something like PathBuf::from_iter(Path::new($ui_dir))

p.s. this $ui_dir isn't used https://docs.rs/marker_uitest/latest/src/marker_uitest/lib.rs.html#33

Alexendoo commented 12 months ago

🤔 wait no, it should be picking it up https://regex101.com/r/fnT6rV/1

xFrednet commented 12 months ago

I've tried your suggestion and it seems to work, at least i have a green CI now

xFrednet commented 12 months ago

Are you overriding the default filters by any chance?

AFAIK, I've only added new filters.

p.s. this $ui_dir isn't used docs.rs/marker_uitest/latest/src/marker_uitest/lib.rs.html#33

Ohhh, yeah, thank you!

xFrednet commented 12 months ago

I appreciate the help, everything seems to work now :D

@oli-obk Should I close this issue, since it was a misconfiguration, or do you think this path normalization should also be in ui_test?

oli-obk commented 12 months ago

Yea I think I also have added workarounds for this issue. We could just normalize the PathBufs that end up getting printed (or used in filters) before doing so.

Alexendoo commented 12 months ago

I don't actually see why this is failing, https://github.com/rust-marker/marker-example-lints/blob/959fdbc6298e57bc4f16cfeee9983e9e36911ef8/tests/uitest.rs passes the test successfully for me on a windows machine. Also passes if I remove the two filters

If I change it to verbose the path is there with the mixed slashes in:

tests/ui\almost_complete_range.rs ... ok
xFrednet commented 12 months ago

That's weird, the windows CI of that branch failed though :thinking: Here is a link to the job:

https://github.com/rust-marker/marker-example-lints/actions/runs/6417999196/job/17424884765

The logs weirdly don't show the actual diff, I just guessed that it was a related problem.

The relevant part of the log ``` Compiling marker_example_lints v0.1.0 (D:\a\marker-example-lints\marker-example-lints) Finished test [unoptimized] target(s) in 53.75s Running unittests src\lib.rs (target\debug\deps\marker_example_lints-d8bfbb505ed81bdc.exe) running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s Running tests\uitest.rs (target\debug\deps\uitest-69dbb741b2fdcaed.exe) marker_rustc_driver 0.3.0 (76e7f1f 2023-10-05) Error: tests failed FAILED TEST: tests/ui\almost_complete_range.rs command: "C:\\Users\\runneradmin\\.rustup\\toolchains\\nightly-2023-08-24-x86_64-pc-windows-msvc\\bin\\marker_rustc_driver.exe" "--error-format=json" "-Aunused" "--out-dir" "\\\\?\\D:\\a\\marker-example-lints\\marker-example-lints\\target\\ui_test\\tests\\ui" "tests/ui\\almost_complete_range.rs" "--edition" "2021" Location: C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\ui_test-0.21.2\src\lib.rs:388:13 error: actual output differed from expected error: test failed, to rerun pass `--test uitest` Execute `cargo test -- -- --bless` to update `tests/ui\almost_complete_range.stderr` to the actual output --- tests/ui\almost_complete_range.stderr Caused by: +++ process didn't exit successfully: `D:\a\marker-example-lints\marker-example-lints\target\debug\deps\uitest-69dbb741b2fdcaed.exe` (exit code: 1) warning: almost complete ascii range --> $DIR/almost_complete_range.rs:9:13 ... 36 lines skipped ... warning: 6 warnings emitted full stderr: warning: almost complete ascii range Warning: --> tests/ui\almost_complete_range.rs:9:13 | 9 | let _ = (b'a'..b'z').contains(&x); | ^^^^^^^^^^^^ help: try: `b'a'..=b'a'` | = note: `#[warn(marker::almost_complete_range)]` on by default warning: almost complete ascii range Warning: --> tests/ui\almost_complete_range.rs:10:13 | 10 | let _ = (b'A'..b'Z').contains(&x); | ^^^^^^^^^^^^ help: try: `b'A'..=b'A'` warning: almost complete ascii range Warning: --> tests/ui\almost_complete_range.rs:11:13 | 11 | let _ = (b'0'..b'9').contains(&x); | ^^^^^^^^^^^^ help: try: `b'0'..=b'0'` warning: almost complete ascii range Warning: --> tests/ui\almost_complete_range.rs:12:13 | 12 | let _ = ('a'..'z').contains(&c); | ^^^^^^^^^^ help: try: `'a'..='a'` warning: almost complete ascii range Warning: --> tests/ui\almost_complete_range.rs:13:13 | 13 | let _ = ('A'..'Z').contains(&c); | ^^^^^^^^^^ help: try: `'A'..='A'` warning: almost complete ascii range Warning: --> tests/ui\almost_complete_range.rs:14:13 | 14 | let _ = ('0'..'9').contains(&c); | ^^^^^^^^^^ help: try: `'0'..='0'` warning: 6 warnings emitted full stdout: FAILURES: tests/ui\almost_complete_range.rs test result: FAIL. 1 failed; ```

I've also tried to test if it was a problem with different line endings or something, but couldn't identify the error :thinking:

oli-obk commented 12 months ago

That diff looks weird. Maybe this is line ending nonsense in git?

https://github.com/oli-obk/ui_test/blob/main/.gitattributes was necessary in this crate due to dogfooding, but it seems to be an issue in general, even with compiletest... time to add it to the readme

Alexendoo commented 12 months ago

There are default filters to remove the \r, so it shouldn't matter if it uses CRLF

oli-obk commented 12 months ago

There are default filters to remove the \r, so it shouldn't matter if it uses CRLF

It does, because the .stderr file will end up having them without the gitattributes file

xFrednet commented 12 months ago

I've added a .gitattributes file in the PR as well, that could have been the fix. That's still weird. I tried printing invisible characters with cat after blessing and didn't see any CLRF, but that could have been after the .gitattributes file :thinking:

Alexendoo commented 12 months ago

.stderr file will end up having them without the gitattributes file

Aha, yeah that's it. unix2dosing the .stderr files give the same output

oli-obk commented 12 months ago

Yea, when changing git attributes like that you need to refresh the worktree (I just did something like rm -r * && git checkout HEAD --hard)

I tried adding stderr filters before, which works, until you bless and get no \r, as then git shows the files to differ even if they don't.

I can probably work around that, too somehow, but the git attributes seemed easier.

I can add something that detects if the only difference is a \r and emit a suggestion to add a gitattributes file

xFrednet commented 12 months ago

I can add something that detects if the only difference is a \r and emit a suggestion to add a gitattributes file

That would be perfect. If users see that and don't use git, they can still manually add a filter, that replaces the line feeds with just a \n.

For testing, it could also be cool, to have an option to print invisible characters as well. Not sure, how easy this would be though and the use cases might be very limited as well :)

oli-obk commented 12 months ago

Yea I tried this before, but it's a bit annoying for this crate's dogfooding