Skip to content

Commit 7009011

Browse files
committed
Auto merge of #78752 - jyn514:html-diff, r=GuillaumeGomez
Give a better error when rustdoc tests fail - Run the default rustdoc against the current rustdoc - Diff output recursively - Colorize diff output Closes #78750. ## Resolved questions - Should this be opt-in instead of on by default? + No - Should this call through to `delta`? That's not a very common program to have installed, but I'm not sure how to do diffs after the fact. Maybe `compiletest` can take a `--syntax-highlighter` parameter or something? + I decided to use `delta` if available and `diff --color` otherwise. It prints a warning if delta isn't installed so you know you can get nicer diffs ## Open questions. - What version of rustdoc would this compare against? Ideally it would compare against `$(git merge-base HEAD origin/master)` - maybe that's feasible if we install those artifacts from CI? - Does it always make sense to compare the tests? Especially for new tests, I'm not sure how useful it would be ... but then again, one of the questions I want to know most as a reviewer is 'did it break before?'. r? `@GuillaumeGomez` cc `@Mark-Simulacrum`
2 parents a1a13b2 + 25a3ffe commit 7009011

File tree

3 files changed

+146
-10
lines changed

3 files changed

+146
-10
lines changed

Diff for: src/tools/compiletest/src/json.rs

+7-4
Original file line numberDiff line numberDiff line change
@@ -153,11 +153,14 @@ fn parse_line(file_name: &str, line: &str, output: &str, proc_res: &ProcRes) ->
153153
if serde_json::from_str::<FutureIncompatReport>(line).is_ok() {
154154
vec![]
155155
} else {
156-
proc_res.fatal(Some(&format!(
157-
"failed to decode compiler output as json: \
156+
proc_res.fatal(
157+
Some(&format!(
158+
"failed to decode compiler output as json: \
158159
`{}`\nline: {}\noutput: {}",
159-
error, line, output
160-
)));
160+
error, line, output
161+
)),
162+
|| (),
163+
);
161164
}
162165
}
163166
}

Diff for: src/tools/compiletest/src/main.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,7 @@ pub fn run_tests(config: Config) {
379379
}
380380
Err(e) => {
381381
// We don't know if tests passed or not, but if there was an error
382-
// during testing we don't want to just suceeed (we may not have
382+
// during testing we don't want to just succeed (we may not have
383383
// tested something), so fail.
384384
//
385385
// This should realistically "never" happen, so don't try to make

Diff for: src/tools/compiletest/src/runtest.rs

+138-5
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,7 @@ pub fn compute_stamp_hash(config: &Config) -> String {
287287
format!("{:x}", hash.finish())
288288
}
289289

290+
#[derive(Copy, Clone)]
290291
struct TestCx<'test> {
291292
config: &'test Config,
292293
props: &'test TestProps,
@@ -1729,7 +1730,7 @@ impl<'test> TestCx<'test> {
17291730
self.config.target.contains("vxworks") && !self.is_vxworks_pure_static()
17301731
}
17311732

1732-
fn compose_and_run_compiler(&self, mut rustc: Command, input: Option<String>) -> ProcRes {
1733+
fn build_all_auxiliary(&self, rustc: &mut Command) -> PathBuf {
17331734
let aux_dir = self.aux_output_dir_name();
17341735

17351736
if !self.props.aux_builds.is_empty() {
@@ -1748,6 +1749,11 @@ impl<'test> TestCx<'test> {
17481749
rustc.arg("--extern").arg(format!("{}={}/{}", aux_name, aux_dir.display(), lib_name));
17491750
}
17501751

1752+
aux_dir
1753+
}
1754+
1755+
fn compose_and_run_compiler(&self, mut rustc: Command, input: Option<String>) -> ProcRes {
1756+
let aux_dir = self.build_all_auxiliary(&mut rustc);
17511757
self.props.unset_rustc_env.clone().iter().fold(&mut rustc, |rustc, v| rustc.env_remove(v));
17521758
rustc.envs(self.props.rustc_env.clone());
17531759
self.compose_and_run(
@@ -2209,7 +2215,17 @@ impl<'test> TestCx<'test> {
22092215

22102216
fn fatal_proc_rec(&self, err: &str, proc_res: &ProcRes) -> ! {
22112217
self.error(err);
2212-
proc_res.fatal(None);
2218+
proc_res.fatal(None, || ());
2219+
}
2220+
2221+
fn fatal_proc_rec_with_ctx(
2222+
&self,
2223+
err: &str,
2224+
proc_res: &ProcRes,
2225+
on_failure: impl FnOnce(Self),
2226+
) -> ! {
2227+
self.error(err);
2228+
proc_res.fatal(None, || on_failure(*self));
22132229
}
22142230

22152231
// codegen tests (using FileCheck)
@@ -2326,15 +2342,131 @@ impl<'test> TestCx<'test> {
23262342
let res = self.cmd2procres(
23272343
Command::new(&self.config.docck_python)
23282344
.arg(root.join("src/etc/htmldocck.py"))
2329-
.arg(out_dir)
2345+
.arg(&out_dir)
23302346
.arg(&self.testpaths.file),
23312347
);
23322348
if !res.status.success() {
2333-
self.fatal_proc_rec("htmldocck failed!", &res);
2349+
self.fatal_proc_rec_with_ctx("htmldocck failed!", &res, |mut this| {
2350+
this.compare_to_default_rustdoc(&out_dir)
2351+
});
23342352
}
23352353
}
23362354
}
23372355

2356+
fn compare_to_default_rustdoc(&mut self, out_dir: &Path) {
2357+
println!("info: generating a diff against nightly rustdoc");
2358+
2359+
let suffix =
2360+
self.safe_revision().map_or("nightly".into(), |path| path.to_owned() + "-nightly");
2361+
let compare_dir = output_base_dir(self.config, self.testpaths, Some(&suffix));
2362+
// Don't give an error if the directory didn't already exist
2363+
let _ = fs::remove_dir_all(&compare_dir);
2364+
create_dir_all(&compare_dir).unwrap();
2365+
2366+
// We need to create a new struct for the lifetimes on `config` to work.
2367+
let new_rustdoc = TestCx {
2368+
config: &Config {
2369+
// FIXME: use beta or a user-specified rustdoc instead of
2370+
// hardcoding the default toolchain
2371+
rustdoc_path: Some("rustdoc".into()),
2372+
// Needed for building auxiliary docs below
2373+
rustc_path: "rustc".into(),
2374+
..self.config.clone()
2375+
},
2376+
..*self
2377+
};
2378+
2379+
let output_file = TargetLocation::ThisDirectory(new_rustdoc.aux_output_dir_name());
2380+
let mut rustc = new_rustdoc.make_compile_args(
2381+
&new_rustdoc.testpaths.file,
2382+
output_file,
2383+
EmitMetadata::No,
2384+
AllowUnused::Yes,
2385+
);
2386+
rustc.arg("-L").arg(&new_rustdoc.aux_output_dir_name());
2387+
new_rustdoc.build_all_auxiliary(&mut rustc);
2388+
2389+
let proc_res = new_rustdoc.document(&compare_dir);
2390+
if !proc_res.status.success() {
2391+
proc_res.fatal(Some("failed to run nightly rustdoc"), || ());
2392+
}
2393+
2394+
#[rustfmt::skip]
2395+
let tidy_args = [
2396+
"--indent", "yes",
2397+
"--indent-spaces", "2",
2398+
"--wrap", "0",
2399+
"--show-warnings", "no",
2400+
"--markup", "yes",
2401+
"--quiet", "yes",
2402+
"-modify",
2403+
];
2404+
let tidy_dir = |dir| {
2405+
let tidy = |file: &_| {
2406+
Command::new("tidy")
2407+
.args(&tidy_args)
2408+
.arg(file)
2409+
.spawn()
2410+
.unwrap_or_else(|err| {
2411+
self.fatal(&format!("failed to run tidy - is it installed? - {}", err))
2412+
})
2413+
.wait()
2414+
.unwrap()
2415+
};
2416+
for entry in walkdir::WalkDir::new(dir) {
2417+
let entry = entry.expect("failed to read file");
2418+
if entry.file_type().is_file()
2419+
&& entry.path().extension().and_then(|p| p.to_str()) == Some("html".into())
2420+
{
2421+
tidy(entry.path());
2422+
}
2423+
}
2424+
};
2425+
tidy_dir(out_dir);
2426+
tidy_dir(&compare_dir);
2427+
2428+
let pager = {
2429+
let output = Command::new("git").args(&["config", "--get", "core.pager"]).output().ok();
2430+
output.and_then(|out| {
2431+
if out.status.success() {
2432+
Some(String::from_utf8(out.stdout).expect("invalid UTF8 in git pager"))
2433+
} else {
2434+
None
2435+
}
2436+
})
2437+
};
2438+
let mut diff = Command::new("diff");
2439+
diff.args(&["-u", "-r"]).args(&[out_dir, &compare_dir]);
2440+
2441+
let output = if let Some(pager) = pager {
2442+
let diff_pid = diff.stdout(Stdio::piped()).spawn().expect("failed to run `diff`");
2443+
let pager = pager.trim();
2444+
if self.config.verbose {
2445+
eprintln!("using pager {}", pager);
2446+
}
2447+
let output = Command::new(pager)
2448+
// disable paging; we want this to be non-interactive
2449+
.env("PAGER", "")
2450+
.stdin(diff_pid.stdout.unwrap())
2451+
// Capture output and print it explicitly so it will in turn be
2452+
// captured by libtest.
2453+
.output()
2454+
.unwrap();
2455+
assert!(output.status.success());
2456+
output
2457+
} else {
2458+
eprintln!("warning: no pager configured, falling back to `diff --color`");
2459+
eprintln!(
2460+
"help: try configuring a git pager (e.g. `delta`) with `git config --global core.pager delta`"
2461+
);
2462+
let output = diff.arg("--color").output().unwrap();
2463+
assert!(output.status.success() || output.status.code() == Some(1));
2464+
output
2465+
};
2466+
println!("{}", String::from_utf8_lossy(&output.stdout));
2467+
eprintln!("{}", String::from_utf8_lossy(&output.stderr));
2468+
}
2469+
23382470
fn get_lines<P: AsRef<Path>>(
23392471
&self,
23402472
path: &P,
@@ -3591,7 +3723,7 @@ pub struct ProcRes {
35913723
}
35923724

35933725
impl ProcRes {
3594-
pub fn fatal(&self, err: Option<&str>) -> ! {
3726+
pub fn fatal(&self, err: Option<&str>, on_failure: impl FnOnce()) -> ! {
35953727
if let Some(e) = err {
35963728
println!("\nerror: {}", e);
35973729
}
@@ -3613,6 +3745,7 @@ impl ProcRes {
36133745
json::extract_rendered(&self.stdout),
36143746
json::extract_rendered(&self.stderr),
36153747
);
3748+
on_failure();
36163749
// Use resume_unwind instead of panic!() to prevent a panic message + backtrace from
36173750
// compiletest, which is unnecessary noise.
36183751
std::panic::resume_unwind(Box::new(()));

0 commit comments

Comments
 (0)