Display correct filename with --test option by GuillaumeGomez · Pull Request #39597 · rust-lang/rust

GuillaumeGomez · 2017-02-06T21:15:34Z

With the current files:

test.rs

pubmod foo;/// This is a Foo;////// ```/// println!("baaaaaar");/// ```#[unstable]pubstructFoo;/// This is a Bar;////// ```/// println!("fooooo");/// ```pubstructBar;

foo.rs

// note the whitespaces/// ```/// println!("foo");/// ```pubfnfoo(){}

It displays:

./build/x86_64-apple-darwin/stage1/bin/rustdoc --test test.rs
running 3 tests
test test.rs - line 13 ... ok
test test.rs - line 5 ... ok
test foo.rs - line 2 ... ok
test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured

And with a markdown file:

# foo
` ``
println!("lol");
` ``
asdjnfasd
asd

It displays:

./build/x86_64-apple-darwin/stage1/bin/rustdoc --test foo.md
running 1 test
test <input> - line 3 ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured

r? @alexcrichton

alexcrichton · 2017-02-06T21:37:47Z

Can you be sure to add a test for this as well?

GuillaumeGomez · 2017-02-06T22:29:06Z

@alexcrichton: So I came up with a solution, does it seem ok to you?

alexcrichton · 2017-02-07T17:18:07Z

@bors: r+

bors · 2017-02-07T17:18:08Z

📌 Commit d92abb3 has been approved by alexcrichton

@alexcrichton

…_file, r=alexcrichton Display correct filename with --test option Fixesrust-lang#39592. With the current files: ```rust pub mod foo; /// This is a Foo; /// /// ``` /// println!("baaaaaar"); /// ``` pub struct Foo; /// This is a Bar; /// /// ``` /// println!("fooooo"); /// ``` pub struct Bar; ``` ```rust // note the whitespaces /// ``` /// println!("foo"); /// ``` pub fn foo() {} ``` It displays: ``` ./build/x86_64-apple-darwin/stage1/bin/rustdoc --test test.rs running 3 tests test test.rs - line 13 ... ok test test.rs - line 5 ... ok test foo.rs - line 2 ... ok test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured ``` ``` ` `` println!("lol"); ` `` asdjnfasd asd ``` It displays: ``` ./build/x86_64-apple-darwin/stage1/bin/rustdoc --test foo.md running 1 test test <input> - line 3 ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured ``` r? @alexcrichton

SergioBenitez · 2017-02-08T04:04:29Z

- &format!("Not found doc test: \"{}\" in {:?}", s, v),
- &res);
+ let path = tmp[0].rsplit("test ").next().unwrap();
+ println!("{:?}", path);


Is this println really supposed to be here?

No, absolutely not! Thanks a lot for notifying it!

SergioBenitez · 2017-02-08T04:06:19Z

I don't know if this PR addresses the entire issue. Another thing that I can't do anymore is run a subset of the doctests by specifying a prefix: cargo test some_module. Doing this currently runs the unit tests, but not doctests, for that prefix. This is another fairly large, breaking regression. Is this fixed by this PR?

This PR also doesn't address another part of #39592: the module/function name being tested is no longer printed. I have no idea what's on line 194 of user.rs, but I do know what the function get_current_user is supposed to do.

GuillaumeGomez · 2017-02-08T09:10:44Z

@bors: r+ r=alexcrichton

bors · 2017-02-08T09:12:04Z

📌 Commit d2f8abf has been approved by alexcrichton

GuillaumeGomez · 2017-02-08T09:14:44Z

@SergioBenitez: cargo test is based on name so if you want to run "specific" doc tests, you should specify the filename instead. Also, the ids of rustdoc were pretty stupid, now at least you know from which file and which line it comes from (and then you can go read the corresponding code directly).

SergioBenitez · 2017-02-08T09:25:12Z

This is not the way things worked before, however, and frankly, I don't believe this makes any sense. There is now a strange inconsistency between cargo test {name} when running doctests vs. unit tests, an inconsistency that didn't exist before. I can't run all of the tests for a single module with a single command anymore. Now, I have to figure out the path to a module, run the module's unit tests, and then run the modules doctests. This is a major usability regression.

I think this is incredibly confusing. What does "the ids of rustdoc were pretty stupid" mean? It's great that I now know the file/line the test is on (by the way, are those numbers actually correct? I'm seeing some strange numbers...), but I also want to know the logical path to the module. Like I said, I have no idea which function/item is in which file/line, and forcing me to check out the file individually is a huge bummer. I want to know the file/line if I'm going to go and fix it, but I want to know the module path when I just want to know what went wrong. Both pieces of information are useful. Nothing is stupid about wanting to have the full picture.

GuillaumeGomez · 2017-02-08T10:24:22Z

The ids were stupid because they generally didn't give much indication. What happens if you have the same function name in multiple traits? Simply something beautiful like this:

func_0
func_1
func_2
func_3
func_4
...

Super useful, indeed. And I don't even speak when you run rustdoc --test on markdown files. No, clearly, this new system is way better. With the fix, all the files will be correctly printed.

@alexcrichton

…_file, r=alexcrichton Display correct filename with --test option Fixesrust-lang#39592. With the current files: ```rust pub mod foo; /// This is a Foo; /// /// ``` /// println!("baaaaaar"); /// ``` pub struct Foo; /// This is a Bar; /// /// ``` /// println!("fooooo"); /// ``` pub struct Bar; ``` ```rust // note the whitespaces /// ``` /// println!("foo"); /// ``` pub fn foo() {} ``` It displays: ``` ./build/x86_64-apple-darwin/stage1/bin/rustdoc --test test.rs running 3 tests test test.rs - line 13 ... ok test test.rs - line 5 ... ok test foo.rs - line 2 ... ok test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured ``` ``` ` `` println!("lol"); ` `` asdjnfasd asd ``` It displays: ``` ./build/x86_64-apple-darwin/stage1/bin/rustdoc --test foo.md running 1 test test <input> - line 3 ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured ``` r? @alexcrichton

Rollup of 11 pull requests - Successful merges: #39462, #39512, #39529, #39557, #39561, #39582, #39583, #39597, #39622, #39624, #39630 - Failed merges:

alexcrichton · 2017-02-08T20:53:56Z

@GuillaumeGomez it seems like both indicators could be inserted in the test name, right? We could have both a small snippet of what file and also the old historical context for what's actually being tested.

SergioBenitez · 2017-02-10T04:43:00Z

The ids were stupid because they generally didn't give much indication. What happens if you have the same function name in multiple traits? Simply something beautiful like this:
func_0
func_1
func_2
func_3
func_4
...
Super useful, indeed.

That's not what Cargo prints out at all. This is what gets printed out:

running 6 tests
test MyTrait::a_0 ... ok
test A::a_0 ... ok
test B::a_0 ... ok
test A::a_0 ... ok
test MyOtherTrait::a_0 ... ok
test B::a_0 ... ok

The logs includes the full path to the trait/function, not just the function name, thus disambiguating. I do agree that the _n is a bit odd, but you get used to it very quickly. I'd personally like to see #n instead: MyTrait::a#0, MyTrait::a#1, and so on. Or maybe even, MyTrait::a test 1, MyTrait::a test 2, and so on. Or not use it at all an disambiguate with the file/line, as I'll show below.

The other issue I have with this new format is that it uses absolute paths; they're just too long. I don't care about the absolute path to my project; I ran cargo test, I'm in the workspace. I think the path should be relative to the current Cargo workspace or Cargo root.

I think something like this would look nice:

test MyTrait::a (src/lib.rs:2) ... ok
test A::a (src/lib.rs:9) ... ok
test B::a (src/lib.rs:33) ... ok
test A::a (src/lib.rs:19) ... ok
test MyOtherTrait::a (src/lib.rs:40) ... ok
test B::a (src/lib.rs:26) ... ok

This is concise, informative, and includes all of the old and new information.

GuillaumeGomez · 2017-02-10T10:20:54Z

Having the two pieces of information could be useful, indeed. I can add it back (without the _n if you don't mind). Could you open an issue for it please?

Display correct filename with --test option
b0803d4

rust-highfive assigned alexcrichton Feb 6, 2017

frewsxcv mentioned this pull request Feb 8, 2017
Rollup of 34 pull requests #39632
Closed

SergioBenitez reviewed Feb 8, 2017
View reviewed changes

Add more test for rustdoc --test
d2f8abf

GuillaumeGomez force-pushed the correct_rustdoc_test_file branch from d92abb3 to d2f8abfCompare February 8, 2017 09:10

frewsxcv mentioned this pull request Feb 8, 2017
Rollup of 11 pull requests #39645
Merged

bors added a commit that referenced this pull request Feb 8, 2017
Auto merge of#39645- frewsxcv:rollup, r=frewsxcv
4379e2f
Rollup of 11 pull requests - Successful merges: #39462, #39512, #39529, #39557, #39561, #39582, #39583, #39597, #39622, #39624, #39630 - Failed merges:

bors merged commit d2f8abf into rust-lang:masterFeb 8, 2017

GuillaumeGomez deleted the correct_rustdoc_test_file branch February 8, 2017 21:05

Uh oh!

Uh oh!

Conversation

GuillaumeGomez commented Feb 6, 2017

test.rs

foo.rs

And with a markdown file:

Uh oh!

alexcrichton commented Feb 6, 2017

Uh oh!

GuillaumeGomez commented Feb 6, 2017

Uh oh!

alexcrichton commented Feb 7, 2017

Uh oh!

bors commented Feb 7, 2017

Uh oh!

SergioBenitezFeb 8, 2017

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomezFeb 8, 2017

Choose a reason for hiding this comment

Uh oh!

SergioBenitez commented Feb 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Feb 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Feb 8, 2017

Uh oh!

GuillaumeGomez commented Feb 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergioBenitez commented Feb 8, 2017

Uh oh!

GuillaumeGomez commented Feb 8, 2017

Uh oh!

alexcrichton commented Feb 8, 2017

Uh oh!

SergioBenitez commented Feb 10, 2017

Uh oh!

GuillaumeGomez commented Feb 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SergioBenitez commented Feb 8, 2017•
edited
Loading

GuillaumeGomez commented Feb 8, 2017•
edited
Loading

GuillaumeGomez commented Feb 8, 2017•
edited
Loading