Skip to content

Commit

Permalink
Make compiletest diffs more useful
Browse files Browse the repository at this point in the history 
- Outside of CI, don't show the full rustc command unless --verbose is passed.
- Outside of CI, only show one of the diff and the full stderr. The heuristic for whether to show the diff is:
  - it takes up less than 10 lines, or
  - it takes up less than a tenth of the total stderr output
- If we show the diff, also show the difference between the normalized output and the actual output for lines which didn't match.

before:
```
failures:

---- [ui] tests/ui/layout/enum.rs stdout ----
Saved the actual stderr to "/home/jyn/src/rust2/build/x86_64-unknown-linux-gnu/test/ui/layout/enum/enum.stderr"
diff of stderr:

-	error: align: AbiAndPrefAlign { abi: Align(2 bytes), pref: $PREF_ALIGN }
+	error: align: AbiAndPrefAlign { abi: Align(2 bytes), pref: $PREF_ALIN }
2	  --> $DIR/enum.rs:9:1
3	   |
4	LL | enum UninhabitedVariantAlign {

The actual stderr differed from the expected stderr.
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args layout/enum.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/home/jyn/src/rust2/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/home/jyn/src/rust2/tests/ui/layout/enum.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/home/jyn/.local/lib/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/home/jyn/src/rust2/vendor" "--sysroot" "/home/jyn/src/rust2/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/home/jyn/src/rust2/build/x86_64-unknown-linux-gnu/test/ui/layout/enum" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/home/jyn/src/rust2/build/x86_64-unknown-linux-gnu/native/rust-test-helpers"
stdout: none
--- stderr -------------------------------
error: align: AbiAndPrefAlign { abi: Align(2 bytes), pref: Align(8 bytes) }
  --> /home/jyn/src/rust2/tests/ui/layout/enum.rs:9:1
   |
LL | enum UninhabitedVariantAlign { //~ERROR: abi: Align(2 bytes)
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: size: Size(16 bytes)
  --> /home/jyn/src/rust2/tests/ui/layout/enum.rs:15:1
   |
LL | enum UninhabitedVariantSpace { //~ERROR: size: Size(16 bytes)
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: abi: ScalarPair(Initialized { value: Int(I8, false), valid_range: 0..=1 }, Initialized { value: Int(I8, false), valid_range: 0..=255 })
  --> /home/jyn/src/rust2/tests/ui/layout/enum.rs:21:1
   |
LL | enum ScalarPairDifferingSign { //~ERROR: abi: ScalarPair
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors
------------------------------------------

failures:
    [ui] tests/ui/layout/enum.rs
```

after:
```
failures:

---- [ui] tests/ui/layout/enum.rs stdout ----
Saved the actual stderr to "/home/jyn/src/rust2/build/x86_64-unknown-linux-gnu/test/ui/layout/enum/enum.stderr"
diff of stderr:

-	error: align: AbiAndPrefAlign { abi: Align(2 bytes), pref: $PREF_ALIGN }
+	error: align: AbiAndPrefAlign { abi: Align(2 bytes), pref: $PREF_ALIN }
2	  --> $DIR/enum.rs:9:1
3	   |
4	LL | enum UninhabitedVariantAlign {

Note: some mismatched output was normalized before being compared
-	error: align: AbiAndPrefAlign { abi: Align(2 bytes), pref: Align(8 bytes) }
-	  --> /home/jyn/src/rust2/tests/ui/layout/enum.rs:9:1
+	error: align: AbiAndPrefAlign { abi: Align(2 bytes), pref: $PREF_ALIGN }

The actual stderr differed from the expected stderr.
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args layout/enum.rs`

error: 1 errors occurred comparing output.

failures:
    [ui] tests/ui/layout/enum.rs
    [ui] tests/ui/proc-macro/load-panic-backtrace.rs
```
  • Loading branch information
@jyn514
jyn514 committed Dec 2, 2024
1 parent 5bbbc09 commit 4682a01
Show file tree
Hide file tree
Showing 3 changed files with 152 additions and 54 deletions.
178 changes: 133 additions & 45 deletions src/tools/compiletest/src/runtest.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use std::sync::Arc;
use std::{env, iter, str};

use anyhow::Context;
use build_helper::ci::CiEnv;
use colored::Colorize;
use regex::{Captures, Regex};
use tracing::*;
Expand All @@ -22,7 +23,7 @@ use crate::common::{
UI_STDERR, UI_STDOUT, UI_SVG, UI_WINDOWS_SVG, Ui, expected_output_path, incremental_dir,
output_base_dir, output_base_name, output_testname_unique,
};
use crate::compute_diff::{write_diff, write_filtered_diff};
use crate::compute_diff::{DiffLine, make_diff, write_diff, write_filtered_diff};
use crate::errors::{self, Error, ErrorKind};
use crate::header::TestProps;
use crate::read2::{Truncated, read2_abbreviated};
Expand Down Expand Up @@ -2292,17 +2293,31 @@ impl<'test> TestCx<'test> {
match output_kind {
TestOutput::Compile => {
if !self.props.dont_check_compiler_stdout {
errors +=
self.compare_output(stdout_kind, &normalized_stdout, &expected_stdout);
errors += self.compare_output(
stdout_kind,
&normalized_stdout,
&proc_res.stdout,
&expected_stdout,
);
}
if !self.props.dont_check_compiler_stderr {
errors +=
self.compare_output(stderr_kind, &normalized_stderr, &expected_stderr);
errors += self.compare_output(
stderr_kind,
&normalized_stderr,
&stderr,
&expected_stderr,
);
}
}
TestOutput::Run => {
errors += self.compare_output(stdout_kind, &normalized_stdout, &expected_stdout);
errors += self.compare_output(stderr_kind, &normalized_stderr, &expected_stderr);
errors += self.compare_output(
stdout_kind,
&normalized_stdout,
&proc_res.stdout,
&expected_stdout,
);
errors +=
self.compare_output(stderr_kind, &normalized_stderr, &stderr, &expected_stderr);
}
}
errors
Expand Down Expand Up @@ -2530,7 +2545,13 @@ impl<'test> TestCx<'test> {
}
}

fn compare_output(&self, stream: &str, actual: &str, expected: &str) -> usize {
fn compare_output(
&self,
stream: &str,
actual: &str,
actual_unnormalized: &str,
expected: &str,
) -> usize {
let are_different = match (self.force_color_svg(), expected.find('\n'), actual.find('\n')) {
// FIXME: We ignore the first line of SVG files
// because the width parameter is non-deterministic.
Expand Down Expand Up @@ -2590,27 +2611,16 @@ impl<'test> TestCx<'test> {
if expected.is_empty() {
println!("normalized {}:\n{}\n", stream, actual);
} else {
println!("diff of {stream}:\n");
if let Some(diff_command) = self.config.diff_command.as_deref() {
let mut args = diff_command.split_whitespace();
let name = args.next().unwrap();
match Command::new(name)
.args(args)
.args([&expected_path, &actual_path])
.output()
{
Err(err) => {
self.fatal(&format!(
"failed to call custom diff command `{diff_command}`: {err}"
));
}
Ok(output) => {
let output = String::from_utf8_lossy(&output.stdout);
print!("{output}");
}
}
} else {
print!("{}", write_diff(expected, actual, 3));
let show_full = self.show_diff(
stream,
&expected_path,
&actual_path,
expected,
actual,
actual_unnormalized,
);
if show_full || self.config.verbose {
println!("{}", ProcRes::render(stream, actual));
}
}
} else {
Expand All @@ -2633,6 +2643,84 @@ impl<'test> TestCx<'test> {
if self.config.bless { 0 } else { 1 }
}

/// Returns whether to show the full stderr/stdout.
fn show_diff(
&self,
stream: &str,
expected_path: &Path,
actual_path: &Path,
expected: &str,
actual: &str,
actual_unnormalized: &str,
) -> bool {
let context_size = 3;
// NOTE: argument order is important, we need `actual` to be on the left so the line number match up when we compare it to `actual_unnormalized` below.
let diff_results = make_diff(actual, expected, context_size);
let diff_lines: usize = diff_results.iter().map(|hunk| hunk.lines.len()).sum();
let show_diff = diff_lines < 10 || diff_lines < expected.lines().count() / 10;
if !show_diff && !CiEnv::is_ci() {
// don't even bother showing a diff if too many lines are changed, it won't be helpful.
let diff_cmd = self.config.diff_command.as_deref().unwrap_or("diff");
println!(
"too many lines changed; hiding diff. use `{diff_cmd} {expected_path:?} {actual_path:?}` to view anyway."
);
return true;
}

println!("diff of {stream}:\n");
if let Some(diff_command) = self.config.diff_command.as_deref() {
let mut args = diff_command.split_whitespace();
let name = args.next().unwrap();
match Command::new(name).args(args).args([expected_path, actual_path]).output() {
Err(err) => {
self.fatal(&format!(
"failed to call custom diff command `{diff_command}`: {err}"
));
}
Ok(output) => {
let output = String::from_utf8_lossy(&output.stdout);
print!("{output}");
}
}
} else {
print!("{}", write_diff(expected, actual, context_size));
}

let (mut mismatches_normalized, mut mismatch_line_nos) = (String::new(), vec![]);
for hunk in diff_results {
let mut line_no = hunk.line_number;
for line in hunk.lines {
if let DiffLine::Resulting(normalized) = line {
mismatches_normalized += &normalized;
mismatches_normalized += "\n";
mismatch_line_nos.push(line_no);
line_no += 1;
}
}
}
let mut mismatches_unnormalized = String::new();
let diff_normalized = make_diff(actual, actual_unnormalized, 0);
for hunk in diff_normalized {
if mismatch_line_nos.contains(&hunk.line_number) {
for line in hunk.lines {
if let DiffLine::Resulting(unnormalized) = line {
mismatches_unnormalized += &unnormalized;
mismatches_unnormalized += "\n";
}
}
}
}

let normalized_diff = make_diff(&mismatches_normalized, &mismatches_unnormalized, 0);
if !normalized_diff.is_empty() {
println!("Note: some mismatched output was normalized before being compared");
// FIXME: respect diff_command
print!("{}", write_diff(&mismatches_unnormalized, &mismatches_normalized, 0));
}

CiEnv::is_ci()
}

fn check_and_prune_duplicate_outputs(
&self,
proc_res: &ProcRes,
Expand Down Expand Up @@ -2749,28 +2837,28 @@ pub struct ProcRes {
}

impl ProcRes {
pub fn print_info(&self) {
fn render(name: &str, contents: &str) -> String {
let contents = json::extract_rendered(contents);
let contents = contents.trim_end();
if contents.is_empty() {
format!("{name}: none")
} else {
format!(
"\
--- {name} -------------------------------\n\
{contents}\n\
------------------------------------------",
)
}
pub fn render(name: &str, contents: &str) -> String {
let contents = json::extract_rendered(contents);
let contents = contents.trim_end();
if contents.is_empty() {
format!("{name}: none")
} else {
format!(
"\
--- {name} -------------------------------\n\
{contents}\n\
------------------------------------------",
)
}
}

pub fn print_info(&self) {
println!(
"status: {}\ncommand: {}\n{}\n{}\n",
self.status,
self.cmdline,
render("stdout", &self.stdout),
render("stderr", &self.stderr),
Self::render("stdout", &self.stdout),
Self::render("stderr", &self.stderr),
);
}

Expand Down
16 changes: 12 additions & 4 deletions src/tools/compiletest/src/runtest/coverage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,12 @@ impl<'test> TestCx<'test> {
let expected_coverage_dump = self.load_expected_output(kind);
let actual_coverage_dump = self.normalize_output(&proc_res.stdout, &[]);

let coverage_dump_errors =
self.compare_output(kind, &actual_coverage_dump, &expected_coverage_dump);
let coverage_dump_errors = self.compare_output(
kind,
&actual_coverage_dump,
&proc_res.stdout,
&expected_coverage_dump,
);

if coverage_dump_errors > 0 {
self.fatal_proc_rec(
Expand Down Expand Up @@ -135,8 +139,12 @@ impl<'test> TestCx<'test> {
self.fatal_proc_rec(&err, &proc_res);
});

let coverage_errors =
self.compare_output(kind, &normalized_actual_coverage, &expected_coverage);
let coverage_errors = self.compare_output(
kind,
&normalized_actual_coverage,
&proc_res.stdout,
&expected_coverage,
);

if coverage_errors > 0 {
self.fatal_proc_rec(
Expand Down
12 changes: 7 additions & 5 deletions src/tools/compiletest/src/runtest/ui.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::collections::HashSet;
use std::fs::OpenOptions;
use std::io::Write;

use build_helper::ci::CiEnv;
use rustfix::{Filter, apply_suggestions, get_suggestions_from_json};
use tracing::debug;

Expand Down Expand Up @@ -100,7 +101,7 @@ impl TestCx<'_> {
)
});

errors += self.compare_output("fixed", &fixed_code, &expected_fixed);
errors += self.compare_output("fixed", &fixed_code, &fixed_code, &expected_fixed);
} else if !expected_fixed.is_empty() {
panic!(
"the `//@ run-rustfix` directive wasn't found but a `*.fixed` \
Expand All @@ -116,10 +117,11 @@ impl TestCx<'_> {
"To only update this specific test, also pass `--test-args {}`",
relative_path_to_file.display(),
);
self.fatal_proc_rec(
&format!("{} errors occurred comparing output.", errors),
&proc_res,
);
self.error(&format!("{} errors occurred comparing output.", errors));
if self.config.verbose || CiEnv::is_ci() {
println!("status: {}\ncommand: {}", proc_res.status, proc_res.cmdline);
}
std::panic::resume_unwind(Box::new(()));
}

let expected_errors = errors::load_errors(&self.testpaths.file, self.revision);
Expand Down

0 comments on commit 4682a01

Please sign in to comment.