Generating a documentation for tests by ifxfrancois · Pull Request #130463 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR add the new option --document-tests to rustdoc.
When this option is set:
- the test modules are not filtered out by rustdoc when generating the documentation,
- the functions marked as
#[test]are collected into a new category calledTestsinstead ofFunctions
Note: tests are often not marked as public so the option --document-private-items may be required to see the tests in the documentation.
A short example of the generated docs can be found here.
Motivation
During the development of embedded software, Quality or certifications teams need a quick overview of the tests of a project. Typical relevant information are what, how and why a test tests.
- What would be a short description of the test.
- How would be a short list of the important tests steps.
- Why would be references to the relevant detailed architecture or requirements documents.
This information can easily be written as markdown in the documentation of the test functions and the documentation be generated with the additional option. The generated documentation can then be provided to relevant teams.
Limitations
There is no integration with cargo and integrations test can not be added to the same documentation as the rest of the crate documentation.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) some time within the next two weeks.
Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:
@rustbot author: the review is finished, PR author should check the comments and take action accordingly@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue
Comment on lines 662 to 701
| ItemKind::FunctionItem(_) |
|---|
| | ItemKind::MethodItem(_, _) |
| | ItemKind::TyMethodItem(_) |
| | ItemKind::TestItem(_) => { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modelling a #[test] fn as a distinct ItemKind seems a bit strange: #[test] fns are still function items, I wonder if you can distinguish non-#[test] fns versus #[test] fns by looking at the existence/absence of the #[test] attribute.
But yeah, rendering docs for tests do seem useful. Though a question that immediately comes to my mind is that do test-affiliated module docs get rendered too? As in e.g. docs on #[cfg(test)] mod tests;. I would think that they do, but I haven't looked closely if they do in this implementation.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they are rendered in this implementation though they are most likely not pub therefore the additional flag --document-private-items may be needed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @jieyouxu, but also I like this difference as it makes it simpler to know what's being manipulated... I suppose it's fine as is for now. :)
Another approach could be to add a bool value saying whether or not it's a test function.
As for cfged out items, unfortunately you will need to pass --cfg test to have them (alongside --document-private-items).
| @@ -2238,6 +2248,7 @@ impl ItemSection { |
|---|
| Self::Unions => "Unions", |
| Self::Enums => "Enums", |
| Self::Functions => "Functions", |
| Self::Tests => "Tests", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Unit tests" instead? Same above.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bikeshed can continue as part of the FCP, I think.
But, for my own 2c, I think it should be "Test cases"
| @@ -186,6 +186,7 @@ pub(super) fn print_item(cx: &mut Context<'_>, item: &clean::Item, buf: &mut Buf |
|---|
| } |
| } |
| clean::FunctionItem(..) | clean::ForeignFunctionItem(..) => "Function ", |
| clean::TestItem(..) => "Test ", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Unit test "
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GuillaumeGomez We also want to call this documentation build on integration tests. If we change this to "Unit Test", the integration tests will also appear as Unit tests.
With a command like cargo rustdoc --test lib_test -- -Z unstable-options --document-tests --document-private-items
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Test case "
I think it's a good idea. Just one thing missing: an entry in the rustdoc book about the command line flag and that it needs to be used with --document-private-items and likely with --cfg test as well.
I think it's a good idea. Just one thing missing: an entry in the rustdoc book about the command line flag and that it needs to be used with
--document-private-itemsand likely with--cfg testas well.
@GuillaumeGomez I don't know about the correct page where to add it, should I add it to the page "Unstable features" or to the page "Command-line arguments".
Sorry should have precised. Since the option is unstable, please put it into "Unstable features".
There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.
You can start a rebase with the following commands:
$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease
The following commits are merge commits:
I think it's a good idea. Just one thing missing: an entry in the rustdoc book about the command line flag and that it needs to be used with
--document-private-itemsand likely with--cfg testas well.
It would be quite easy for rustdoc to automaticly set the test cfg when --document-tests is passed, in the same vain as done with the doc cfg.
| // Add the doc cfg into the doc build. |
|---|
| cfgs.push("doc".to_string()); |
This comment has been minimized.
I think it's a good idea. Just one thing missing: an entry in the rustdoc book about the command line flag and that it needs to be used with
--document-private-itemsand likely with--cfg testas well.It would be quite easy for rustdoc to automaticly set the
testcfg when--document-testsis passed, in the same vain as done with thedoccfg.
// Add the doc cfg into the doc build. cfgs.push("doc".to_string());
Yes it's easy to do. The question is more: "should we do it?".
| @@ -229,7 +229,8 @@ pub(crate) fn create_config( | |
|---|---|
| if proc_macro_crate { vec![CrateType::ProcMacro] } else { vec![CrateType::Rlib] }; | |
| let resolve_doc_links = | |
| if *document_private { ResolveDocLinks::All } else { ResolveDocLinks::Exported }; | |
| let test = scrape_examples_options.map(|opts | opts.scrape_tests).unwrap_or(false); |
| let test = | |
| scrape_examples_options.map(|opts | opts.scrape_tests).unwrap_or(false) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea. Just one thing missing: an entry in the rustdoc book about the command line flag and that it needs to be used with
--document-private-itemsand likely with--cfg testas well.It would be quite easy for rustdoc to automaticly set the
testcfg when--document-testsis passed, in the same vain as done with thedoccfg.
// Add the doc cfg into the doc build. cfgs.push("doc".to_string()); Yes it's easy to do. The question is more: "should we do it?".
In this implementation, ' --cfg test` is already passed automatically here.
It may not be the cleanest way to pass it.
Having it passed automatically is convenient, I think it would be a bit counter-intuitive if the option "document-tests" did not actually document tests except if an additional flag is passed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's an important thing to discuss as it might not be obvious for users. I would personally pass --cfg test as I wouldn't expect an option to modify the cfgs.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be good to not set --cfg test automatically in rustdoc.
I can see that in the future we might want to create a cargo wrapper for generating documentation of tests. In this case cargo could provide a user friendly way using only a single command or flag which would set all the required rustdoc flags. Still there would be full control over all flags on the rustdoc level.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case cargo could provide a user friendly way using only a single command or flag which would set all the required rustdoc flags. Still there would be full control over all flags on the rustdoc level.
If this feature were being added in a vacuum, I might agree. But we already have a ton of cases where it's Rustc, not Cargo, that takes care of deciding all the cfg options (except feature=, of course).
For example, rustc --test sets cfg(test), rustc --target sets the target cfgs, rustdoc sets cfg(doc), and rustdoc --test sets cfg(doctest). The tool itself makes these decisions, not Cargo.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.
You can start a rebase with the following commands:
$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease
The following commits are merge commits (since this message was last posted):
This comment has been minimized.
This comment was marked as off-topic.
This comment has been minimized.
Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
label
Comment on lines 533 to 545
| let path: String = path |
|---|
| .segments |
| .iter() |
| .map(|s |
| if s.ident.name == kw::PathRoot { |
| "" |
| } else { |
| s.ident.name.as_str() |
| } |
| }) |
| .intersperse("::") |
| .collect(); |
| if path == "test::TestDescAndFn" { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let path: String = path |
|---|
| .segments |
| .iter() |
| .map(|s |
| if s.ident.name == kw::PathRoot { |
| "" |
| } else { |
| s.ident.name.as_str() |
| } |
| }) |
| .intersperse("::") |
| .collect(); |
| if path == "test::TestDescAndFn" { |
| let path_names = path |
| .segments |
| .iter() |
| .map(|s |
| s.ident.name |
| }); |
| if path_names.eq([sym::test, sym::TestDescAndFn]) { |
I think, to follow this suggestion, you'd need to add TestDescAndFn to https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_span/symbol.rs.html#169
| @@ -2238,6 +2248,7 @@ impl ItemSection { |
|---|
| Self::Unions => "Unions", |
| Self::Enums => "Enums", |
| Self::Functions => "Functions", |
| Self::Tests => "Tests", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bikeshed can continue as part of the FCP, I think.
But, for my own 2c, I think it should be "Test cases"
The new option --document-tests is unstable and documented as such.
In order to use it is needed to add --cfg test and in case the tests
are not marked public to add --document-private-items.
The implementation hide the auto generate main test function and
constants.
@rfcbot concern motivation
During the development of embedded software, Quality or certifications teams need a quick overview of the tests of a project.
I'm not really convinced of this motivation. This seems perfect for a domain specific tool, perhaps one consuming rustdoc JSON output.
If we are going down this path I would expect a lot more work on what test documentation might look like, how it should be presented, etc, and probably an RFC.
My subsequent two concerns somewhat reflect my issue with this motivation; as something written with a rather domain specific motivation, it overindexes on a particular way of testing Rust code.
@rfcbot concern doctests
What about doctests? Different projects have different conventions here: some projects have doctests as their primary method of testing, some have unit/integration tests that cover everything important and just run doctests as a way of keeping the examples working.
@rfcbot concern integration-tests
What about integration tests? These are not handled here at all.
Why would doctests be a problem? They’re always included in the documentation.
Why would doctests be a problem? They’re always included in the documentation.
Because the stated goal is about having a "quick overview" of the tests here. Doctests don't show up that way.
Plus, with such a motivation, you probably want to document your tests, and doctests cannot be documented. This is a situation of tension between the dual purposes of doctests: as tests, and as examples.
Thank you very much for the feedback!
I agree, that the motivation is quite narrow at the moment.
There might be more use cases, but we see it as immediately useful to industries seeking certification of their software (e.g. automotive).
For us JSON output that we can process with other tools would be fine, but that would still need a change in rustdoc to generate this JSON output.
Regarding your other concerns:
- integration tests work in theory, but they are compiled in separate compilation units. Because of this, our current implementation can only generate documentation for a single test in isolation. This is not really useful and would require some other solution or tooling to combine the documentation. Maybe this could be achieved with cargo, but we did not look into that (yet).
- doctests are more complicated. As you already said, there is currently no way for them to be documented. The only thing that comes to my mind at the moment would be to use
#![doc = "some documentation"]inside the doctest, but I am not very convinced by my own idea. If we go for a more general solution we should definitively check for better ways to handle doctests.
How would you suggest we move forward with this topic? Should we adapt this PR so it just generates the JSON output but no HTML? Or would you say we should create an RFC first either way?
Rustdoc already produces json output (--output-format json). You should be able to get the output you need by pairing that with --document-private-items.
I'm noticing two issues:
- It doesn't seem to include attributes on functions (this should be fixed; rustdoc-HTML does include e.g. the
no_mangleattribute in its displayed output) - Even if it did, it would filter out
#[test], due to this code. Given that that attribute is mostly only on private items I think it's fine to have that be one of the displayed doc attributes.
So for next steps I would recommend experimenting with the existing tooling, and trying to patch up gaps in it.
I just tried to generate JSON output for with tests but I could not get it to work. I also tried using rustdoc with --cfg test which included test modules, but not the tests themselves.
I believe the problem is that the HIR only contains tests if --test is passed to rustc. As far as I can see, rustdoc only passes --test to rustc when scraping examples from tests. In your opinion, what would be the best way to allow passing --test to rustc through rustdoc? A new CLI flag or some other approach?
- It doesn't seem to include attributes on functions (this should be fixed; rustdoc-HTML does include e.g. the
no_mangleattribute in its displayed output)- Even if it did, it would filter out
#[test], due to this code. Given that that attribute is mostly only on private items I think it's fine to have that be one of the displayed doc attributes.
Looking at the HIR generated by rustc (cargo rustc -- -Z unpretty=hir --test) it looks like rustc is doing quite a lot of transformations on the tests. In particular, the #[test] does not appear in the HIR. This would mean the tooling working with the JSON output would have to do something similar to what this PR is doing with regards to recovering the tests from TestDescAndFn constants.
No, I think your problem is that private items are stripped by default. Try with --document-private-items too.
No, I think your problem is that private items are stripped by default. Try with
--document-private-itemstoo.
I did try with --document-private-items, --cfg test and --test. None of those produces a JSON file which contains any mention of my example test (When passing --test no JSON file is generated at all).
My current test setup is a crate with a lib.rs which just contains the following test:
/// A test with documentation. #[test] fn some_documented_test() { assert_eq!(2 + 2, 4); }
I am running rustdoc +nightly --document-private-items -Z unstable-options --output-format json src/lib.rs
The resulting JSON does not contain some_documented_test at all.
I see, it seems like a new option is needed to make it work then.
No, I don't see why a new option is needed, there is no reason that document-private-items cannot have this behavior already.
Nothing here needs new functionality, it just needs tweaks to existing functionality.
Euh yes... What I had in mind was "--document-private-items should list #[test] functions". Not sure how I came writing my previous comment...
Do you mean --document-private-items should pass --test to rustc or that it should do more work to just add the tests?
The problem is that when given the --test option, rustc creates a ne main function which runs all tests, a TestDescAndFn constant for each test and an array containing these constants. Maybe even more things, that are not in the top of my head at the moment.
Would including these artifacts be ok for the JSON output? Or should some work be done to remove them?
I am also not sure, if all users of --document-private-items would always want the tests to be included...
We mean that if there is a visible #[test] function, it should be listed.
ok, I will try to find a way to achieve this without causing extra artifacts.
This should still only apply for JSON output, right?
I don't see why? Generally they're put behind cfg(test). If not, then I suppose people will discover that when documenting wth --document-private-items.