Port clippy away from compiletest to ui_test

[oli-obk]

Reasons to do this:

• runs completely on stable Rust
• is easier to extend with new features
• has its own dogfood test suite, so changes can be tested in the ui_test repo
• supports dependencies from crates.io without having to manually fiddle with command line flags
• supports ui-cargo, ui, ui-toml out of the box, no need to find and run the tests ourselves

One thing that is a big difference to compiletest is that if a test emits any error, you need to mark all of them with //~ ERROR: annotations. Since some clippy tests did have (sometimes wrong) annotations, I fixed those tests, but changed no others to have annotations.

TODO:

• check that this still works as a subtree in the rustc repo

changelog: none

Note: at present the latest changes needed for clippy are only available as a git dependency, but I expect to publish a new crates.io version soon

[rustbot]

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

[flip1995]

r? @Alexendoo Since you already took some time looking into compiletest and using a distributed binary from the Rust repo in rust-lang/rust#103266. What do you think about this alternative?

[bors]

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

[Manishearth]

cc @rust-lang/clippy

(Could you split the commit between the one that fixes the tests and the one that updates the runner?)

[Alexendoo]

Nice! I'll take a look at it

For those testing locally you'll want

ui_test = { git = "https://github.com/oli-obk/ui_test", branch = "aux_build" }

Here's some stuff I ran into:

clippy_test_deps/Cargo.lock doesn't get generated for us

---- compile_test stdout ----
   Compiler flags: ["--error-format=json", "--edition=2021", "--emit=metadata", "-Aunused", "-Zui-testing"]
   Building test dependencies...
thread 'compile_test' panicked at 'called `Result::unwrap()` on an `Err` value: failed to compile dependencies:
stderr:
    Updating crates.io index
error: the lock file /home/alex/rust/clippy/clippy_test_deps/Cargo.lock needs to be updated but --locked was passed to prevent this
If you want to try to generate the lock file without accessing the network, remove the --locked flag and use --offline instead.


stdout:

Location:
    /home/alex/.cargo/git/checkouts/ui_test-2b82183a391bb05c/d900958/src/dependencies.rs:59:9', tests/compile-test.rs:83:36

The second is that there's no progress output, I don't know how compiletest-rs manages it, but it prints the test progress without having to pass --nocapture

Related is that sometimes the tests aren't saying why they have failed, e.g. if I put //@typo in one of the test files I get the output

running 3 tests
test rustfix_coverage_known_exceptions_accuracy ... ok
test ui_cargo_toml_metadata ... ok
error: test failed, to rerun pass `--test compile-test`

Caused by:
  process didn't exit successfully: `/home/alex/rust/clippy/target/debug/deps/compile_test-bbe576cdf579a984` (exit status: 1)

What's particularly odd there is that passing -- --nocapture does show the reason for the failure

tests/ui/asm_syntax.rs FAILED:
command: "parse comments"

Could not parse comment in tests/ui/asm_syntax.rs:1 because unknown command `typo`

[oli-obk]

clippy_test_deps/Cargo.lock doesn't get generated for us

you need to env BLESS=1 to bless lock files. I will change that blessing to whatever scheme you'd like, but I think clippy had no scheme for blessing while running tests before?

edit: committed the lockfile, I forgot because it was in .gitignore 😆

[oli-obk]

The second is that there's no progress output, I don't know how compiletest-rs manages it, but it prints the test progress without having to pass --nocapture

ah, yea I can do that. Btw: do y'all want dots or filenames by default? I can make it so CI prints filenames and local running prints dots.

[Manishearth]

Looks like some tests are broken

There should be no changes to blessed files, no?

[bors]

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

[Alexendoo]

There's some tests that still have // old-style-headers:

tests/ui-internal/custom_ice_message.rs
tests/ui/almost_complete_range.rs
tests/ui/asm_syntax.rs
tests/ui/author/blocks.rs
tests/ui/cast_size_32bit.rs
tests/ui/cast_size.rs
tests/ui/crashes/ice-4968.rs
tests/ui/crashes/ice-7410.rs
tests/ui/crate_level_checks/entrypoint_recursion.rs
tests/ui/crate_level_checks/no_std_main_recursion.rs
tests/ui/def_id_nocore.rs
tests/ui/empty_loop_no_std.rs
tests/ui/entry.rs
tests/ui/enum_clike_unportable_variant.rs
tests/ui/expect_tool_lint_rfc_2383.rs
tests/ui/fn_to_numeric_cast_32bit.rs
tests/ui/fn_to_numeric_cast.rs
tests/ui/macro_use_imports_expect.rs
tests/ui/macro_use_imports.rs
tests/ui/manual_assert.rs
tests/ui/missing_doc.rs
tests/ui/must_use_unit.rs
tests/ui/needless_parens_on_range_literals.rs
tests/ui/needless_splitn.rs
tests/ui/no_mangle_with_rust_abi.rs
tests/ui/non_octal_unix_permissions.rs
tests/ui/numbered_fields.rs
tests/ui/redundant_clone.rs
tests/ui/to_digit_is_some.rs
tests/ui/transmute_32bit.rs
tests/ui/transmute_64bit.rs
tests/ui/uninlined_format_args_panic.rs
tests/ui/wildcard_imports_2021.rs
tests/ui/wildcard_imports.rs

From rg '^// *(edition|revisions|\w+-)' tests -t rust if you want something more locally clickable

[oli-obk]

oh oops

error: 'cargo-clippy' is not installed for the toolchain 'nightly-2023-02-25-x86_64-unknown-linux-gnu'
To install, run rustup component add clippy --toolchain nightly-2023-02-25-x86_64-unknown-linux-gnu

I guess for the ui-cargo tests I better build cargo-clippy first. Or should I revert to the previous behaviour of collecting the .rs files and running clippy-driver on them?

[Alexendoo]

It won't be in the toolchain but cargo-clippy should be built alongside clippy-driver, so something like

config.program.set_file_name(if cfg!(windows) {
    "cargo-clippy.exe"
} else {
    "cargo-clippy"
});

would hopefully work

[Alexendoo]

should I revert to the previous behaviour of collecting the .rs files and running clippy-driver on them?

I'd say actually running cargo-clippy would be nicer if we can get it to work well

[oli-obk]

Ah the remaining failure is a flakiness where ui-cargo causes collisions between crates of the same name. Obviously tests should be allowed to have the same name and not conflict. This needs a fix in ui_test.

[Alexendoo]

On Windows some backslashes in paths are left behind:

actual output differed from expected
--- tests\ui\approx_const.stderr
+++ <stderr output>
 warning: approximate value of `f{32, 64}::consts::E` found
-  --> $DIR/approx_const.rs:4:16
+  --> $DIR\approx_const.rs:4:16
    |
 LL |     let my_e = 2.7182;

[oli-obk]

CI is green, though if someone could check whether it works on windows that would be great.

There's some follow-ups I want to do, like changing the entire test suite back to -Dwarnings, but that requires touching 150 tests or so, so I'd prefer not to do that in this PR.

I'll update the usage instructions now, too, as it's not necessary anymore to run cargo dev bless, instead the stderr files are updated by enabling the BLESS env var. Any opinions on the scheme for this? name of env var? different scheme like cargo test -- --bless?

[Alexendoo]

Windows errors:

ui:

--- tests\ui\track-diagnostics.stderr
+++ <stderr output>
... 3 lines skipped ...
 LL | const S: A = B;
    |              ^ expected `A`, found `B`
--Ztrack-diagnostics: created at compiler/rustc_infer/src/infer/error_reporting/mod.rs:LL:CC
+-Ztrack-diagnostics: created at compiler/rustc_infer\src\infer\error_reporting\mod.rs:LL:CC

--- tests\ui\crashes\ice-7868.stderr
+++ <stderr output>
 warning: unsafe block missing a safety comment
-  --> $DIR/auxiliary/ice-7868-aux.rs:2:5
+  --> $DIR/auxiliary\ice-7868-aux.rs:2:5
    |
 LL |     unsafe { 0 };

ui-cargo needs regex escaping as well:

regex parse error:
    \\?\D:\Code\Rust\clippy
          ^^
error: unrecognized escape sequence

ui-internal:

actual output differed from expected
--- tests\ui-internal\custom_ice_message.stderr
+++ <stderr output>
-thread '<unnamed>' panicked at 'Would you like some help with that?', clippy_lints/src/utils/internal_lints/produce_ice.rs
+thread '<unnamed>' panicked at 'Would you like some help with that?', clippy_lints/src\utils\internal_lints\produce_ice.rs
 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[Alexendoo]

The ui-toml suite isn't picking up any tests as this is Path::ends_with

Suggested change

	|path| path.ends_with(".rs"),
	|path| path.extension() == Some("rs".as_ref()),

[Alexendoo]

I think this'd still need to go through regex::escape, as canonical windows paths contain ?, e.g. \\?\D:\Code\Rust\clippy\

[Manishearth]

I went through all of the changes.

Overall, here's what I see:

• a bunch of changes wrt the precise compiletest flags
• some minor changes to output wrt $DIR and such, as well as line numbering changes
• some new .fixed files

(Would be nice to have separate commits for these but it's less of a problem now that i've reviewed them; but for future commits might be worth doing commits incrementally and merging in master so the review history is preserved)

This all seems okay.

I also found two bugs, one is the "multiple test flags" one, and one is where some proc macro errors turn up for a couple tests.

So I think we're quite close to landing this. I'd also be open to landing this as is with an issue filed for fixing those buggy tests.

(edit: lol code changed whilst I was reviewing: the "multiple test flags" one is fixed now)

[Manishearth]

this seems like an unintentional deletion

[oli-obk]

I don't think this has worked at all, the other test in the same file doesn't have a .stderr file either. I'm guessing compiletest-rs didn't care that there was an extraneous file lying around if the test passes without output?

[Manishearth]

ah, seems fair. ty!

[oli-obk]

You're all too fast ^^ I just pushed it to get it to my windows machine 😅

[oli-obk]

Would it maybe be possible to make the //~ ERROR checks optional? I've been looking at ui_test for a different project, where I would like the option not to annotate every test with these

yes, that's either Yolo or Fail { require_patterns: false } mode: https://docs.rs/ui_test/0.11.1/ui_test/enum.Mode.html

[xFrednet]

yes, that's either Yolo or Fail { require_patterns: false } mode: https://docs.rs/ui_test/0.11.1/ui_test/enum.Mode.html

From the documentation it sounds like Yolo requires the annotations. And Fail, is fine with missing annotations, as along as the compilation errors. Is my understanding correct?

If so, I was basically looking for Yolo { require_patterns: false}

[oli-obk]

Yolo is what clippy uses. So it is similar to Fail { require: false }, but also allows passing tests.

[oli-obk]

Ok, CI is happy, testing against rustc CI now, so the next sync doesn't have a nasty surprise

[oli-obk]

The sync is currently blocked on rust-lang/rust#112708

Should we wait until that is resolved and we can actually try this in a sync, or are y'all fine with me just being on on-call for the next sync to fix it?

[Manishearth]

I'm ok with merging this now and you and @flip1995 coordinating the sync.

[Alexendoo]

Does cargo dev bless work after this? If not we can save some upstream headaches by migrating the current test setup to pick up on the BLESS env variable before this is merged + update bootstrap to use it ahead of time - https://github.com/rust-lang/rust/blob/6f8c27ae89dfd32895419d7ef5b89844bcad1bcd/src/bootstrap/test.rs#L810C1-L810C1

It probably wouldn't block sync but would block any upstream PR that needs to update clippy tests

[oli-obk]

Does cargo dev bless work after this?

no, there is no more way of blessing after running tests. You can only bless while running tests. The command for this is cargo bless or cargo uibless for just blessing the regular ui test suite (but not the toml, cargo, ... ones).

[flip1995]

I wouldn't block this on the sync. Let's get this merged. 🚀

[Manishearth]

I feel like we have alignment from three team members, so

@bors r+

Thanks for your diligence on this!!

[bors]

📌 Commit 0a87ce8 has been approved by Manishearth

It is now in the queue for this repository.

[bors]

⌛ Testing commit 0a87ce8 with merge 15ed281...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing 15ed281 to master...