New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add information in case of markdown block code test failure #36320

Merged
merged 15 commits into from Feb 4, 2017

Conversation

Projects
None yet
7 participants
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Sep 7, 2016

@jonathandturner

This comment has been minimized.

Copy link
Contributor

jonathandturner commented Sep 8, 2016

rustdoc is a tools team thing.

cc @alexcrichton for a reviewer

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 8, 2016

Since this is a very small change, I didn't think it was needed, my bad!

r? @alexcrichton

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:rustdoc_test_info branch from 68ea42f to 5f8b5ef Sep 8, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 8, 2016

Thanks for the PR! Could you provide some snippets of what the output looks like here?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 9, 2016

Sure, here you go:

running 1 test
test test_1_0 ... FAILED

failures:

---- test_1_0 stdout ----
    thread 'test_1_0' panicked at 'test compiled while it wasn't supposed to:

```compile_fail
println!("hoho");
```'

', src/librustdoc/test.rs:270
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    test_1_0

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 12, 2016

Thanks! My only worry here is that failing tests may get quite verbose as examples can sometimes be relatively long. Thoughts @rust-lang/tools?

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 15, 2016

Ah nice. Figuring out which code blocks doc tests represent is a huge pain.

It seems to me that the most useful info here is "src/librustdoc/test.rs:270". I wonder if we could simply rename the tests to incorporate that information. I don't think there's any particular limit on what the test runners will accept as a test name, so that exact string would probably work.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 16, 2016

@brson: I didn't understand. What do you want exactly?

Also, to be sure we say the same thing, "src/librustdoc/test.rs:270" is where the error comes from in librustdoc. It's because of panic!, so totally useless to locate the invalid code block.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 20, 2016

Oh, huh. So you are right that that line number is useless. I was thinking that was being printed intentionally by rustdoc as the source location of the test case.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 20, 2016

I'm not sure what I think of the PR as written. I think the output is kinda weird, with the entire test content being part of the panic message, and thus producing that weird formatting of the panic location that I misunderstood. I guess I'm still inclined to a solution that points you to the original source location rather than printing the entire test case.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 20, 2016

@brson: It has to handle both markdown and rust files. And in our error-code-list.md (generated), having a line number doesn't help much from my opinion, I think it's preferrable to just display the failing test and let the users handle it.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 23, 2016

Any news?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 26, 2016

@GuillaumeGomez I believe what @brson means is that the panic message should indicate where the test was defined rather than the test itself. I'm also inclined to agree as it should be less wordy

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 27, 2016

@alexcrichton: In a lot of cases, it's not possible. I take as example the error code list. First, we generate a markdown file and then we run rustdoc on it. What's the point to give the line in this case? It'll be easier to just return the code responsible for the error (people will certainly find out more quickly this way).

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 27, 2016

Can we at least give the line number in the markdown file?

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:rustdoc_test_info branch from 5f8b5ef to bcb1d9f Sep 28, 2016

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 28, 2016

@GuillaumeGomez For the general case of documentation test cases, I think it is more convenient to give a line-number, for most (all?) rustdoc uses I'm aware of out-of-tree, rustdoc is testing documentation that users wrote directly, and can point precisely where the error is in the file they wrote. The generated error code list is an in-tree matter, and is not as important as the ergonomics for our users.

Though I still agree the user experience here can be better, I'm yet unconvinced that printing the entire test case is the best solution.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 28, 2016

I proposed to truncate to 20 lines (or any other number).

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 28, 2016

So here are my arguments in favor of showing the code: we don't just say "you have an error in [file:line}", we also display the error. I think it should be the same and I'll go for this solution. After, I can always truncate the code sample, but that's secondary.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 28, 2016

Can we start out with file names and line numbers and see what that looks like?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 2, 2016

So I added filename and line number, which gives us the following output:

thread 'yolo_0' panicked at 'test compiled while it wasn't supposed to:

Error on test.rs:10

 ```compile_fail
let y = 0;
    ``` // <- I added 4 spaces before because gh parsing failure

', src/librustdoc/test.rs:286

I still think we should keep the test code and truncate it to 20 lines or something along the lines. I really don't want to remove it.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:rustdoc_test_info branch from d7a4494 to 5edce3a Oct 2, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 4, 2016

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

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:rustdoc_test_info branch from 5edce3a to d81f6a4 Oct 4, 2016

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 9, 2016

I truncated the output to 10 lines. So, does it seem good now?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 13, 2016

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:rustdoc_test_info branch from 088d7c1 to b7f1d7a Feb 3, 2017

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 3, 2017

@alexcrichton: I updated to last cargo master again. I r+ it, don't hesitate to r- if needed!

@bors: r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 3, 2017

📌 Commit b7f1d7a has been approved by GuillaumeGomez

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 3, 2017

@bors: r- r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 3, 2017

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #39418
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 3, 2017

📌 Commit b7f1d7a has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 3, 2017

⌛️ Testing commit b7f1d7a with merge 63bffd7...

bors added a commit that referenced this pull request Feb 3, 2017

Auto merge of #36320 - GuillaumeGomez:rustdoc_test_info, r=alexcrichton
Add information in case of markdown block code test failure

r? @steveklabnik

cc @jonathandturner
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 3, 2017

💔 Test failed - status-travis

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 3, 2017

Looks like 2 builds failed:

The run-pass-fulldeps failure may be spurious, although I haven't seen it before. The timeout during doc-tests on core though seems particularly worrisome because of how many changes were made to rustdoc here.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 3, 2017

@bors: retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 4, 2017

⌛️ Testing commit b7f1d7a with merge c781fc4...

bors added a commit that referenced this pull request Feb 4, 2017

Auto merge of #36320 - GuillaumeGomez:rustdoc_test_info, r=alexcrichton
Add information in case of markdown block code test failure

r? @steveklabnik

cc @jonathandturner
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing c781fc4 to master...

@bors bors merged commit b7f1d7a into rust-lang:master Feb 4, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:rustdoc_test_info branch Feb 4, 2017

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