Give a better error when rustdoc tests fail #78752

jyn514 · 2020-11-04T22:50:43Z

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

jyn514 · 2020-11-04T22:53:24Z

Example output:

src/tools/compiletest/tidy-rustdoc.sh

src/tools/compiletest/src/runtest.rs

src/tools/compiletest/tidy-rustdoc.sh

jyn514 · 2020-11-06T23:03:18Z

Oh another thing - this compares the output eagerly, as the tests are still running. Maybe it would be better to wait until the end?

It turns out there's no way to do this, since all the tests are boxed and have no way to return anything. I don't think the giant refactor would be worth it, I made delta non-interactive instead (no pager).

src/tools/compiletest/src/runtest.rs

pickfire · 2020-11-07T16:15:59Z

Should this be opt-in instead of on by default?

I am not in the team but I would say it should be opt-out basis (on by default), for other contributors coming in they could see better error messages in the CI.

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?

Way not take the options from git? Or just do git diff?

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?'.

Feels like a git bisect question? Maybe we need to allow users to do a bisect? Not sure, I never did one for rust due to compilation time.

jyn514 · 2020-11-07T17:18:50Z

Way not take the options from git? Or just do git diff?

Git diff doesn't help because these files aren't tracked by git, they're created by rustdoc. Instead I fell back to diff --color if delta wasn't installed.

Feels like a git bisect question? Maybe we need to allow users to do a bisect? Not sure, I never did one for rust due to compilation time.

Right, yeah I don't think it makes any sense to use bisect since it takes so long to compile things. The other rustdoc has to be pre-compiled from somewhere.

Another thing I found is that if you compare to beta rustdoc the diff is very noisy, because there are unrelated changes to the files in the meantime. Maybe I should only compare HTML files and not JS? Here's an example of what I mean:

comparing: /home/joshua/rustc/build/x86_64-unknown-linux-gnu/test/rustdoc/wrapping/theme.js ⟶   /home/joshua/rustc/build/x86_64-unknown-linux-gnu/test/rustdoc/wrapping.nightly/theme.js
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

1
var themes=document.getElementById("theme-choices");var themePicker=document.getElementById("theme-picker");function showThemeButtonState(){themes.style.display="block";themePicker.style.borderBottomRightRadius="0";themePicker.style.borderBottomLeftRadius="0"}function hideThemeButtonState(){themes.style.display="none";themePicker.style.borderBottomRightRadius="3px";themePicker.style.borderBottomLeftRadius="3px"}function switchThemeButtonState(){if(themes.style.display==="block"){hideThemeButtonState()}els→
\ No newline at end of file
var themes=document.getElementById("theme-choices");var themePicker=document.getElementById("theme-picker");function showThemeButtonState(){themes.style.display="block";themePicker.style.borderBottomRightRadius="0";themePicker.style.borderBottomLeftRadius="0"}function hideThemeButtonState(){themes.style.display="none";themePicker.style.borderBottomRightRadius="3px";themePicker.style.borderBottomLeftRadius="3px"}function switchThemeButtonState(){if(themes.style.display==="block"){hideThemeButtonState()}els→
\ No newline at end of file

jyn514 · 2020-11-07T18:11:37Z

And it doesn't work for aux-build either :/

Fixed in 90fa9b6.

Oh another thing - this compares the output eagerly, as the tests are still running. Maybe it would be better to wait until the end?

Fixed in 9c160ed.

jyn514 · 2020-11-07T18:13:55Z

Ok, I think this is ready for review.

pickfire · 2020-11-08T02:08:31Z

Git diff doesn't help because these files aren't tracked by git, they're created by rustdoc. Instead I fell back to diff --color if delta wasn't installed.

No, I wasn't saying if the files are tracked by git. I am just saying to use git diff, it could be used as a normal diff.

jyn514 · 2020-11-08T02:17:11Z

That doesn't look like any git diff I've ever seen.

$ echo test > f
$ echo test 1 | git diff f -
fatal: option '-' must come before non-option arguments
$ git diff f <(echo test 1)
diff --git a/f b/f
deleted file mode 100644
index 9daeafb9864..00000000000
--- a/f
+++ /dev/null
@@ -1 +0,0 @@
-test
diff --git a/dev/fd/63 b/dev/fd/63
new file mode 120000
index 00000000000..0b022da6c30
--- /dev/null
+++ b/dev/fd/63
@@ -0,0 +1 @@
+pipe:[2557594]
\ No newline at end of file
$ echo test 1 > g
$ git diff f g
$ cat f
test
$ cat g
test 1

GuillaumeGomez · 2020-11-08T14:07:17Z

Should this be opt-in instead of on by default?

Is there any reason it shouldn't? Personally I think we should so that if there is any issue, we'll know right away.

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 didn't even know about it. I think we should avoid relying on external tools if possible (or only provide it through an option).

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.

👍

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?'.

Tests are mostly to prevent regressions. So unless there is one, I'm not sure if it'd useful... I'm waiting for others' opinion there too. :)

Nemo157 · 2020-11-08T16:10:11Z

That doesn't look like any git diff I've ever seen.

That's what it looks like with core.pager = delta configured (and the theming of such that @pickfire uses), seems like a pretty good way to implicitly detect any improved CLI diff viewer the user might use.

dandavison · 2020-11-11T03:41:36Z

@dandavison What do you think?

I don't have the full context here yet but in general I'm happy for delta to be used like this. It does seem like, in the absence of a Unix-ey standard for specifying ones preferred diff viewer, that the user's choice of git pager, if discernible, might be a reasonable choice. I'm happy for you to default to delta if it appears to be available.

In the longer term I'll just note that there is an open issue for delta to be refactored as a Rust crate, which would give you another option for a default without worrying about the user's environment. But I'm not sure yet when that will be done! dandavison/delta#317

But git is the good way to know if someone is using delta as their pager. Maybe we could parse some environment variables or check git config -l for that but I don't think we should implicitly choose the pager for the users.

I didn't see this mentioned but just in case it helps there's also git config --get core.pager.

jyn514 · 2020-11-14T07:45:43Z

Ok, I used this to use the default git pager instead of hardcoding delta or diff --color.

- Run the default rustdoc against the current rustdoc - Diff output recursively - Colorize diff output

- remove unused args - Fix formatting - Improve naming - Fix typo

jyn514 · 2020-11-18T05:13:25Z

@GuillaumeGomez is this waiting on something from me?

GuillaumeGomez · 2020-11-18T12:35:39Z

Well, I was wondering about the bash script. If you want, I can make the update in a follow-up PR? Otherwise it looks good to me so if it's ready for you, r=me.

jyn514 · 2020-11-21T04:03:12Z

I think this is ready to go; I moved the shell script to Rust and this uses git config --get core.pager to determine the pager.

GuillaumeGomez · 2020-11-21T18:46:23Z

@bors: r+

bors · 2020-11-21T18:46:25Z

📌 Commit 25a3ffe has been approved by GuillaumeGomez

bors · 2020-11-22T01:09:10Z

⌛ Testing commit 25a3ffe with merge 7009011...

pickfire · 2020-11-22T02:59:45Z

src/tools/compiletest/src/runtest.rs

+            if self.config.verbose {
+                eprintln!("using pager {}", pager);
+            }
+            let output = Command::new(pager)


Can pager be like diff -u (contain arguments)?

No, Command::new runs it as if it were in quotes.

bors · 2020-11-22T03:36:27Z

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 7009011 to master...

Don't abort rustdoc tests if `tidy` isn't installed Follow-up to rust-lang#78752. Before: ``` Check compiletest suite=rustdoc mode=rustdoc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu) running 396 tests ..................................................2020-11-23T12:12:37.735649Z ERROR compiletest::runtest: fatal error, panic: "failed to run tidy - is it installed? - No such file or directory (os error 2)" F................................................. 100/396 .................................................................................................... 200/396 .................................................................................................... 300/396 ...............................i...............2020-11-23T12:15:00.271271Z ERROR compiletest::runtest: fatal error, panic: "failed to run tidy - is it installed? - No such file or directory (os error 2)" F................................................ ``` After: ``` Check compiletest suite=rustdoc mode=rustdoc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu) running 4 tests .FFF failures: ---- [rustdoc] rustdoc/fn-pointer-arg-name.rs stdout ---- error: htmldocck failed! status: exit code: 1 command: "/usr/bin/python" "/home/joshua/rustc/src/etc/htmldocck.py" "/home/joshua/rustc/build/x86_64-unknown-linux-gnu/test/rustdoc/fn-pointer-arg-name" "/home/joshua/rustc/src/test/rustdoc/fn-pointer-arg-name.rs" stdout: ------------------------------------------ ------------------------------------------ stderr: ------------------------------------------ 4: `@has` check failed `XPATH PATTERN` did not match // `@has` - '//*[`@class="rust` fn"]' 'pub fn f(callback: fn(len: usize, foo: u32))' Encountered 1 errors ------------------------------------------ info: generating a diff against nightly rustdoc failed to run tidy - is it installed? - Permission denied (os error 13) failed to run tidy - is it installed? - Permission denied (os error 13) # a diff without running `tidy` ``` r? `@GuillaumeGomez`

jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Nov 4, 2020

rust-highfive assigned GuillaumeGomez Nov 4, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2020

This comment has been minimized.

Sign in to view

jyn514 marked this pull request as draft November 4, 2020 23:11

jyn514 force-pushed the html-diff branch 2 times, most recently from 278be9b to 5b1bd12 Compare November 4, 2020 23:41

pickfire reviewed Nov 5, 2020

View reviewed changes

src/tools/compiletest/tidy-rustdoc.sh Outdated Show resolved Hide resolved

pickfire reviewed Nov 5, 2020

View reviewed changes

src/tools/compiletest/src/runtest.rs Show resolved Hide resolved

pickfire reviewed Nov 5, 2020

View reviewed changes

src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved

pickfire reviewed Nov 5, 2020

View reviewed changes

src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Nov 5, 2020

View reviewed changes

src/tools/compiletest/tidy-rustdoc.sh Outdated Show resolved Hide resolved

Nemo157 reviewed Nov 5, 2020

View reviewed changes

src/tools/compiletest/tidy-rustdoc.sh Outdated Show resolved Hide resolved

Nemo157 reviewed Nov 5, 2020

View reviewed changes

src/tools/compiletest/tidy-rustdoc.sh Outdated Show resolved Hide resolved

jyn514 mentioned this pull request Nov 6, 2020

Use real Spans when resolving intra-doc links #78797

Closed

pickfire reviewed Nov 7, 2020

View reviewed changes

src/tools/compiletest/src/runtest.rs Show resolved Hide resolved

jyn514 marked this pull request as ready for review November 7, 2020 18:14

jyn514 force-pushed the html-diff branch from 9c160ed to 727813f Compare November 14, 2020 07:45

jyn514 added 7 commits November 14, 2020 02:48

Give a better error when rustdoc tests fail

341eb6d

- Run the default rustdoc against the current rustdoc - Diff output recursively - Colorize diff output

Address review comments

c0eedc0

- remove unused args - Fix formatting - Improve naming - Fix typo

Fix bugs

acd6ce2

Fall back to diff if delta isn't installed

975471c

Fix tests with auxiliary docs

619880e

Capture stdout and stderr of diff so they'll be printed at the end

e6e4a0a

Use default git pager instead of hard-coding delta

4d44d77

Move from bash to rust

25a3ffe

jyn514 force-pushed the html-diff branch from 727813f to 25a3ffe Compare November 21, 2020 04:01

GuillaumeGomez approved these changes Nov 21, 2020

View reviewed changes

bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2020

pickfire reviewed Nov 22, 2020

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 22, 2020

bors merged commit 7009011 into rust-lang:master Nov 22, 2020

rustbot added this to the 1.50.0 milestone Nov 22, 2020

jyn514 deleted the html-diff branch November 22, 2020 03:38

jyn514 mentioned this pull request Nov 24, 2020

Don't abort rustdoc tests if tidy isn't installed #79370

Merged

jyn514 mentioned this pull request Mar 16, 2021

Move compiletest out of tree #82946

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give a better error when rustdoc tests fail #78752

Give a better error when rustdoc tests fail #78752

jyn514 commented Nov 4, 2020 •

edited

Loading

jyn514 commented Nov 4, 2020

This comment has been minimized.

This comment has been minimized.

jyn514 commented Nov 6, 2020

pickfire commented Nov 7, 2020 •

edited

Loading

jyn514 commented Nov 7, 2020

jyn514 commented Nov 7, 2020 •

edited

Loading

jyn514 commented Nov 7, 2020

pickfire commented Nov 8, 2020

jyn514 commented Nov 8, 2020

GuillaumeGomez commented Nov 8, 2020

Nemo157 commented Nov 8, 2020

dandavison commented Nov 11, 2020

jyn514 commented Nov 14, 2020

jyn514 commented Nov 18, 2020

GuillaumeGomez commented Nov 18, 2020

jyn514 commented Nov 21, 2020

GuillaumeGomez commented Nov 21, 2020

bors commented Nov 21, 2020

bors commented Nov 22, 2020

pickfire Nov 22, 2020

jyn514 Nov 22, 2020

bors commented Nov 22, 2020

Give a better error when rustdoc tests fail #78752

Give a better error when rustdoc tests fail #78752

Conversation

jyn514 commented Nov 4, 2020 • edited Loading

Resolved questions

Open questions.

jyn514 commented Nov 4, 2020

This comment has been minimized.

This comment has been minimized.

jyn514 commented Nov 6, 2020

pickfire commented Nov 7, 2020 • edited Loading

jyn514 commented Nov 7, 2020

jyn514 commented Nov 7, 2020 • edited Loading

jyn514 commented Nov 7, 2020

pickfire commented Nov 8, 2020

jyn514 commented Nov 8, 2020

GuillaumeGomez commented Nov 8, 2020

Nemo157 commented Nov 8, 2020

dandavison commented Nov 11, 2020

jyn514 commented Nov 14, 2020

jyn514 commented Nov 18, 2020

GuillaumeGomez commented Nov 18, 2020

jyn514 commented Nov 21, 2020

GuillaumeGomez commented Nov 21, 2020

bors commented Nov 21, 2020

bors commented Nov 22, 2020

pickfire Nov 22, 2020

Choose a reason for hiding this comment

jyn514 Nov 22, 2020

Choose a reason for hiding this comment

bors commented Nov 22, 2020

jyn514 commented Nov 4, 2020 •

edited

Loading

pickfire commented Nov 7, 2020 •

edited

Loading

jyn514 commented Nov 7, 2020 •

edited

Loading