Skip to content
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

**Run** should run doctests #4317

Closed
matklad opened this issue May 5, 2020 · 10 comments · Fixed by #4320
Closed

**Run** should run doctests #4317

matklad opened this issue May 5, 2020 · 10 comments · Fixed by #4320
Labels
E-has-instructions Issue has some instructions and pointers to code to get started E-medium

Comments

@matklad
Copy link
Member

matklad commented May 5, 2020

Relevant code:

@matklad matklad added E-medium E-has-instructions Issue has some instructions and pointers to code to get started labels May 5, 2020
@bnjjj
Copy link
Contributor

bnjjj commented May 5, 2020

Hmm seems not so easy, in fact regarding these issues on cargo rust-lang/cargo#6424 and rust-lang/cargo#6669 the --all-targets seems to not test all targets :(

Then a proposal should be to add a new command suggest to just launch doc test with --doc flag. What do you think ?

@matklad
Copy link
Member Author

matklad commented May 5, 2020

I think the linked cargo issues are irrelevant here? We should just generate cargo test --doc -- name::of::the::test command (ie, just adding --doc to what we do already).

@bnjjj
Copy link
Contributor

bnjjj commented May 5, 2020

Maybe I didn't understood the issue but your linked source code seems to be linked to cargo test --package my_package_name (https://github.com/rust-analyzer/rust-analyzer/blob/d1c1c01309cff76f6ba60c845500f67a1ca6cec6/crates/rust-analyzer/src/main_loop/handlers.rs#L403) and then if we add --doc it won't test unit tests anymore.

@edwin0cheng
Copy link
Member

One of the disadvantages to using cargo test --doc --name::of::the::test is it only support library target:

--doc
    Test only the library’s documentation. This cannot be mixed with other target options.

Maybe we could use rustdoc --test --test-args="name::of::the::test" src/main.rs ?

@matklad
Copy link
Member Author

matklad commented May 5, 2020

I think doc tests outside of the --lib are a niche use-case to begin with, so I'd start with only supporting --doc and doing something custom only when we get a bug report.

@matklad
Copy link
Member Author

matklad commented May 5, 2020

@bnjjj this is about running a single test, which goes via this code path.

@edwin0cheng
Copy link
Member

--bin is common use case too :) , But of course I think starting from simplest is right.

@bjorn3
Copy link
Member

bjorn3 commented May 5, 2020

According to that same page doc tests are only tested for lib targets, which is not true. This is probably outdated documentation.

When no target selection options are given, cargo test will build the following targets of the selected packages:
[...]

  • doc tests for the lib target

@matklad
Copy link
Member Author

matklad commented May 5, 2020

Hm, but doc tests just don't work in --bin?

rust-lang/rust#50784

@bjorn3
Copy link
Member

bjorn3 commented May 5, 2020

I assumed they work.

bnjjj added a commit to bnjjj/rust-analyzer that referenced this issue May 5, 2020
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
bnjjj added a commit to bnjjj/rust-analyzer that referenced this issue May 5, 2020
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bors bors bot closed this as completed in f0411ff May 5, 2020
@bors bors bot closed this as completed in #4320 May 5, 2020
zbsz pushed a commit to zbsz/rust-analyzer that referenced this issue May 10, 2020
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-has-instructions Issue has some instructions and pointers to code to get started E-medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants