Skip to content

Show only stderr diff when a ui test fails - #47185

Merged
bors merged 2 commits into
rust-lang:masterfrom
ritiek:ui-test-failed-output
Jan 12, 2018
Merged

Show only stderr diff when a ui test fails#47185
bors merged 2 commits into
rust-lang:masterfrom
ritiek:ui-test-failed-output

Conversation

@ritiek

Copy link
Copy Markdown
Contributor

Addresses #46826.

This PR will print the normalized output if expected text is empty otherwise it will just print the diff.

Should we also show a few (actual == expected) lines above & below when displaying the diff? What about indicating line numbers as well so one can quickly check mismatch lines in .stderr file?

@ritiek

Copy link
Copy Markdown
ContributorAuthor

cc @oli-obk

Comment threadsrc/tools/compiletest/src/runtest.rs Outdated
match diff {
diff::Result::Left(l) => println!("-{}", l),
diff::Result::Right(r) => println!("+{}", r),
_ => {},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we still show 3 lines of unchanged output to give the context?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Do you mean we should keep diff::Result::Both()? I don't quite get it, could you please explain a bit more?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

something like

diff::Result::Both(t, _)if context_lines_count <= 3 => {println!(" {}", t);
context_lines_count += 1;}

Mainly to emulate the diff -u display style:

 / | (CastTy::Ptr(_), CastTy::Int(_)) |
3 lines before { | (CastTy::FnPtr, CastTy::Int(_)) =>
\ | bcx.ptrtoint(llval, ll_t_out),
| - (CastTy::Int(_), CastTy::Ptr(_)) =>
| - bcx.inttoptr(llval, ll_t_out),
| + (CastTy::Int(_), CastTy::Ptr(_)) => {
| + let usize_llval = bcx.intcast(llval, bcx.ccx.isize_ty(), signed);
| + bcx.inttoptr(usize_llval, ll_t_out)
| + }
/ | (CastTy::Int(_), CastTy::Float) =>
3 lines after { | cast_int_to_float(&bcx, signed, llval, ll_t_in, ll_t_out),
\ | (CastTy::Float, CastTy::Int(IntTy::I)) =>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks, I get it now. I don't think this will be as straight forward and as @durka mentioned:

Sounds like reinventing features from existing diff libraries where there might be a crate that can do it nicely!

I checked out rustfmt and it looks like it already contains some functions we'll need src/tools/rustfmt/src/bin/rustfmt-format-diff.rs.

I tested it out by copying make_diff() and other depending parts to runtest.rs and modifying compare_output() with:

fn compare_output(&self,kind:&str,actual:&str,expected:&str) -> usize{if actual == expected {return0;}if expected.is_empty(){println!("normalized {}:\n{}\n", kind, actual);}else{println!("diff of {}:\n", kind);let results = make_diff(expected, actual,3);for result in results {for line in result.lines{match line {DiffLine::Expected(e) => println!("+{}", e),DiffLine::Context(c) => println!(" {}", c),DiffLine::Resulting(r) => println!("-{}", r),}}}}
...

Here is an example generated diff:

...
---- [ui] ui/issue-10969.rs stdout ----
diff of stderr:
| ^
error[E0618]: expected function, found `i32`
-fun with .stderr
+ --> $DIR/issue-10969.rs:16:5
|
16 | i(); //~ERROR expected function, found `i32`
| ^^^
The actual stderr differed from the expected stderr.
...

What do you guys think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That looks nice; it'd look even better with some line numbers :)

What happens if there are multiple hunks? i.e., multiple distinct parts that changed?

@shepmastershepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 6, 2018
@ritiek
ritiekforce-pushed the ui-test-failed-output branch 3 times, most recently from c224f8a to 9cfd5b3CompareJanuary 6, 2018 05:57
@ritiek

Copy link
Copy Markdown
ContributorAuthor

I've made changes to show line numbers. Here is how the diff looks with committed changes:

Details
---- [ui] ui/issue-10969.rs stdout ----
diff of stderr:
5 | ^^^
6 |
7	note: defined here
+ --> $DIR/issue-10969.rs:11:9
8 |
9	11 | fn func(i: i32) {
10 | ^
12	error[E0618]: expected function, found `i32`
13 --> $DIR/issue-10969.rs:16:5
14 |
+	16 | i(); //~ERROR expected function, found `i32`
-	16 | i(); //~ERROR expected a function, I found `i32`!!
16 | ^^^
17 |
18	note: defined here
19 --> $DIR/issue-10969.rs:15:9
-	i is `i32`, not a function just like I mentioned before
21 |
22	15 | let i = 0i32;
23 | ^
The actual stderr differed from the expected stderr.

My modified issue-10969.stderr:

Details
 1 error[E0618]: expected function, found `i32`
2 --> $DIR/issue-10969.rs:12:5
3 |
4 12 | i(); //~ERROR expected function, found `i32`
5 | ^^^
6 |
7 note: defined here
8 |
9 11 | fn func(i: i32) {
10 | ^
11 12 error[E0618]: expected function, found `i32`
13 --> $DIR/issue-10969.rs:16:5
14 |
15 16 | i(); //~ERROR expected a function, I found `i32`!!
16 | ^^^
17 |
18 note: defined here
19 --> $DIR/issue-10969.rs:15:9
20 i is `i32`, not a function just like I mentioned before
21 |
22 15 | let i = 0i32;
23 | ^
24 25 error: aborting due to 2 previous errors

@ritiek
ritiekforce-pushed the ui-test-failed-output branch from 9cfd5b3 to 5f84181CompareJanuary 6, 2018 06:09
@ritiek
ritiekforce-pushed the ui-test-failed-output branch from 5f84181 to 4054030CompareJanuary 6, 2018 06:13
@nikomatsakis

Copy link
Copy Markdown
Contributor

r? @nikomatsakis

@nikomatsakis

Copy link
Copy Markdown
Contributor

This looks awesome =)

@nikomatsakis

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Jan 11, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit 4054030 has been approved by nikomatsakis

@kennytmkennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 12, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Jan 12, 2018
…omatsakis
Show only stderr diff when a ui test fails
Addresses rust-lang#46826.
This PR will print the normalized output if expected text is empty otherwise it will just print the diff.
Should we also show a few (actual == expected) lines above & below when displaying the diff? What about indicating line numbers as well so one can quickly check mismatch lines in .stderr file?
@kennytmkennytm mentioned this pull request Jan 12, 2018
bors added a commit that referenced this pull request Jan 12, 2018
@bors
bors merged commit 4054030 into rust-lang:masterJan 12, 2018
@bors

bors commented Jan 12, 2018

Copy link
Copy Markdown
Collaborator

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ritiek@nikomatsakis@bors@kennytm@shepmaster