-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Generating a documentation for tests #130463
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: master
Are you sure you want to change the base?
Conversation
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 (
|
Some changes occurred in src/librustdoc/clean/types.rs cc @camelid Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @jsha |
src/librustdoc/clean/types.rs
Outdated
ItemKind::FunctionItem(_) | ||
| ItemKind::MethodItem(_, _) | ||
| ItemKind::TyMethodItem(_) | ||
| ItemKind::TestItem(_) => { |
There was a problem hiding this comment.
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] fn
s 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 cfg
ed out items, unfortunately you will need to pass --cfg test
to have them (alongside --document-private-items
).
src/librustdoc/html/render/mod.rs
Outdated
@@ -2238,6 +2248,7 @@ impl ItemSection { | |||
Self::Unions => "Unions", | |||
Self::Enums => "Enums", | |||
Self::Functions => "Functions", | |||
Self::Tests => "Tests", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Unit test "
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
@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:
The following commits are merge commits: |
It would be quite easy for rustdoc to automaticly set the Lines 206 to 207 in 82d17a4
|
This comment has been minimized.
This comment has been minimized.
Yes it's easy to do. The question is more: "should we do it?". |
src/librustdoc/core.rs
Outdated
@@ -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) || *document_tests; |
There was a problem hiding this comment.
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-items
and likely with--cfg test
as 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 thedoc
cfg.
Lines 206 to 207 in 82d17a4
// 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.
There was a problem hiding this comment.
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 cfg
s.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly relevant MCP rust-lang/compiler-team#785
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, rust-lang/compiler-team#785 has now entered FCP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting with 1.85.0, the test cfg is consider to be a "userspace" config despite being also set by rustc and should be managed by the build system itself.
☔ The latest upstream changes (presumably #130724) made this pull request unmergeable. Please resolve the merge conflicts. |
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:
The following commits are merge commits (since this message was last posted): |
0bef9fe
to
f8de95c
Compare
This comment has been minimized.
This comment has been minimized.
f8de95c
to
fa3e84f
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #131573) made this pull request unmergeable. Please resolve the merge conflicts. |
This will need another review by the reviewer. |
☔ The latest upstream changes (presumably #134550) made this pull request unmergeable. Please resolve the merge conflicts. |
b8fb053
to
c94e982
Compare
☔ The latest upstream changes (presumably #134590) made this pull request unmergeable. Please resolve the merge conflicts. |
9183070
to
cd61af9
Compare
☔ The latest upstream changes (presumably #135031) made this pull request unmergeable. Please resolve the merge conflicts. |
cd61af9
to
965e60c
Compare
This comment has been minimized.
This comment has been minimized.
965e60c
to
aea4a22
Compare
☔ The latest upstream changes (presumably #135947) made this pull request unmergeable. Please resolve the merge conflicts. |
aea4a22
to
d4cf95c
Compare
☔ The latest upstream changes (presumably #137425) made this pull request unmergeable. Please resolve the merge conflicts. |
d4cf95c
to
d12b7b5
Compare
☔ The latest upstream changes (presumably #137914) made this pull request unmergeable. Please resolve the merge conflicts. |
d12b7b5
to
964cf00
Compare
☔ The latest upstream changes (presumably #138021) made this pull request unmergeable. Please resolve the merge conflicts. |
964cf00
to
da5e555
Compare
☔ The latest upstream changes (presumably #138630) made this pull request unmergeable. Please resolve the merge conflicts. |
da5e555
to
14f00fc
Compare
src/librustdoc/visit_ast.rs
Outdated
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" { |
There was a problem hiding this comment.
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
src/librustdoc/html/render/mod.rs
Outdated
@@ -2238,6 +2248,7 @@ impl ItemSection { | |||
Self::Unions => "Unions", | |||
Self::Enums => "Enums", | |||
Self::Functions => "Functions", | |||
Self::Tests => "Tests", |
There was a problem hiding this comment.
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.
14f00fc
to
4f4be34
Compare
@rfcbot fcp merge |
Team member @notriddle has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
@rfcbot concern motivation
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. |
☔ The latest upstream changes (presumably #139881) made this pull request unmergeable. Please resolve the merge conflicts. |
This PR add the new option
--document-tests
to rustdoc.When this option is set:
#[test]
are collected into a new category calledTests
instead 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.
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.