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

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

Open
xFrednet opened this issue Oct 7, 2023 · 17 comments
Labels
enhancement New feature or request

Comments

@xFrednet
Copy link

xFrednet commented Oct 7, 2023

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 #89

@oli-obk
Copy link
Owner

oli-obk commented Oct 7, 2023

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

/// Uses a heuristic to find backslashes in windows style paths
, which we could use to cover your situation, too, once we know what the issue is.

@Alexendoo
Copy link
Contributor

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
Copy link
Contributor

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

@xFrednet
Copy link
Author

xFrednet commented Oct 7, 2023

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

@xFrednet
Copy link
Author

xFrednet commented Oct 7, 2023

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
Copy link
Author

xFrednet commented Oct 7, 2023

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
Copy link
Owner

oli-obk commented Oct 7, 2023

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
Copy link
Contributor

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
Copy link
Author

xFrednet commented Oct 7, 2023

That's weird, the windows CI of that branch failed though 🤔 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:
+++ <stderr output>
  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 🤔

@oli-obk
Copy link
Owner

oli-obk commented Oct 7, 2023

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
Copy link
Contributor

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

@oli-obk
Copy link
Owner

oli-obk commented Oct 7, 2023

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
Copy link
Author

xFrednet commented Oct 7, 2023

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 🤔

@Alexendoo
Copy link
Contributor

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

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

@xFrednet xFrednet changed the title Simplify/Support $DIR replacement for windows. Normalize line endings in .stderr/.stdout files if there is no .gitattributes file Oct 7, 2023
@oli-obk
Copy link
Owner

oli-obk commented Oct 8, 2023

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
Copy link
Author

xFrednet commented Oct 8, 2023

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
Copy link
Owner

oli-obk commented Oct 8, 2023

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

@oli-obk oli-obk added the enhancement New feature or request label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants