Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

Refactor tests to use explicit fixtures #44

Merged
merged 12 commits into from
Jan 16, 2018
Merged

Refactor tests to use explicit fixtures #44

merged 12 commits into from
Jan 16, 2018

Conversation

killercup
Copy link
Member

While this is just a quick draft, these tests use rustfix as a library and showcase how it might be used in clippy's test suite to check that suggestions can be applied. (It could e.g. work as an extension to compiletest.)

While this is just a quick draft, these tests use rustfix as a library
and showcase how it might be used in clippy's test suite to check that
suggestions can be applied. (It could e.g. work as an extension to
compiletest.)
@killercup killercup requested a review from oli-obk January 3, 2018 22:06
Just run it with `env RUSTFIX_TEST_RECORD_JSON=true cargo test`
One of my favorites! Isn't it great that clippy use get rind of one `&`
per clippy run?
There are tests with multiple JSON documents (one per line)
Apparently, it's hard to figure out if files were added/deleted to a
directory in a build script. Oh well.
@@ -7,6 +7,16 @@ use std::collections::HashSet;
pub mod diagnostics;
use diagnostics::{Diagnostic, DiagnosticSpan};

pub fn get_suggestions_from_json(input: &str, only: &HashSet<String>) -> Vec<Suggestion> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should note that this expects rustc's JSON output. Cargo puts each document in a {"message": rustc_doc} wrapper

trace!("{:?}", sol);
for r in sol.replacements {
info!("replaced.");
trace!("{:?}", r);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful for debugging: env RUST_LOG=everything=trace

@killercup
Copy link
Member Author

killercup commented Jan 16, 2018

r? @oli-obk

I hope I have some time over the next weekends to continue to hack on this and maybe even adopt it as an addition to compiletest if that's something you'd like to see. As rustfix depends on serde, and serde uses proc-macros, it sadly won't be possibleeasy to use in the rustc repo right now.

(My not-so-secret goal is to show the vscode-rust plugin fixing clippy lints at FOSDEM. For that, I primarily need a list of lints whose suggestions work as autofixes and some time to hack on RLS to load the clippy plugin.)

cc @Manishearth re: usage in clippy
cc @laumann re: usage in compiletest

@Manishearth
Copy link
Member

This is pretty neat! Working this into compiletest would be great for clippy.

I don't have an explicit list of lints for you, but it would be nice to get rust-lang/rust#39254 implemented (it's pretty straightforward given that nothing is stabilized) so that we can tag these suggestions ourselves

@killercup
Copy link
Member Author

Yeah, rust-lang/rust#39254 would be great to have.

I don't have an explicit list of lints for you

My goal here right now is to supply a tool that we can use to build such a list :)

@oli-obk
Copy link
Collaborator

oli-obk commented Jan 16, 2018

Hmm... I started a review of this on some pc...

TLDR: yes please! The tests look awesome.

As rustfix depends on serde, and serde uses proc-macros

uhm... should be fine on stage2, right?

I wanted to have rustc diagnostic json parsing in a separate crate anyway, so compiletest, rustfix and other tools can reuse it.

WRT this PR, looks good to me, the old fixtures could probably be left in, but they are not really worth their hazzle.

@killercup
Copy link
Member Author

the old fixtures could probably be left in, but they are not really worth their hazzle.

The CLI asserts are actually fine to have, I just wanted to move away from submodules and testing against crates. I also had some shim to run this via cargo (basically run cargo-inti in a tmpdir, copy source to main.rs, run cargo fix --yolo), but it was a total hack and didn't add much value overall.

@killercup
Copy link
Member Author

Alright, let's land this now before I leave this hanging for another week. Feel free to continue discussion stuff here, though.

@killercup killercup merged commit 17507cf into master Jan 16, 2018
@oli-obk oli-obk deleted the refactor/tests branch January 16, 2018 11:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants