search

rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.31k stars 1.53k forks source link

Add a feature to enable/disable external crates in uitests #8206

Open xFrednet opened 2 years ago

xFrednet commented 2 years ago

Description

In #8179 two new dependencies for our UI-tests have been added, futures and tokio. This increases the pipeline runtime by about 3 minutes. The additional time is mostly spent on compiling, from what I can tell. For local tests, I would like to have external crates like these disabled by default and only enable them if I actually develop on related lints and in the pipeline.

This is how I imagine it:

cargo uitest

This runs all tests which only require Clippy. uitests with external crates are ignored on this case

cargo uitest --features=external-dependencies

Runs all uitest, including those that need external dependencies. This would be the default option in the pipeline

Version

No response

Additional Labels

cc: @flip1995 you're our CI wizard :mage:. Maybe you have some thoughts on the matter :upside_down_face:

flip1995 commented 2 years ago

cc: @flip1995 you're our CI wizard :mage:. Maybe you have some thoughts on the matter :upside_down_face:

Adding more features that block tests is a tradeoff. If the average contributor breaks such a test, it will only be caught in CI, which can be annoying to the contributor. So this should only be done to tests that usually don't break. I think that is the case here, so from that perspective I think we can do this.

The other side is, that tests and code that hides behind a feature flag is not tested in rustc. (But we could change that and maybe should)

I don't like a 3min CI time increase, but I don't think we can do much here if we want to lint external crates.

camsteffen commented 2 years ago

rustc has two separate test groups ui and ui-fulldeps. It's not exactly the same use case but I think we could do a similar thing - put all tests that have dependencies in a separate group of tests. Perhaps the non-dependency tests will run faster if we build them without all the --extern flags. I think I'd rather avoid a feature flag because any time you change which features you use everything re-compiles.

flip1995 commented 2 years ago

That sounds good. Should we name this ui-external then? I wouldn't reuse ui-fulldeps if it is not the same, to avoid confusion.

blyxyas commented 1 year ago

@rustbot claim