--show-coverage json by GuillaumeGomez · Pull Request #66472 · 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
Conversation57 Commits6 Checks0 Files changed
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 }})
The purpose of this change is to be able to use it as a tool in docs.rs in order to provide some more stats to crates' owners. Eventually even create a badge or something along the line.
☔ The latest upstream changes (presumably #54733) made this pull request unmergeable. Please resolve the merge conflicts.
I don't have enough experience with rustdoc to review this, sorry :)
Ah my bad, since you commented I thought you felt up for this. ;)
r? @kinnison
What is this intended to be used for?
It seems weird to reuse --output-format for this because the current output of --show-coverage is definitely not HTML. How about modifying --show-coverage to take and argument so we'd have something like --show-coverage table and --show-coverage json?
@ollie27 No, I prefer it this way for this simple reason that I'm working on putting back the json generation generally. That'll make more sense in a broader way.
When I ran ./x.py test --stage 1 --keep-stage 1 src/test/rustdoc-ui I got a significant number of:
thread 'rustc' panicked at 'called Result::unwrap() on an Err value: Utf8Error { valid_up_to: 9, error_len: Some(1) }', src/libcore/macros/mod.rs:23:13
Including when trying to run the coverage/json.rs which this PR includes.
What am I doing wrong?
Well if this is going to use --output-format then it needs another variant like --output-format table for the current output of --show-coverage. --show-coverage --output-format html should be an error because the output is not HTML.
Oh also; it's a request coming from @QuietMisdreavus. :)
The motivation for a PR should be in the PR description and/or commit messages. Based on #58154 (comment) I'm going to assume that the JSON that this outputs is going to be used by some other tool in which case it doesn't need to be prettified, the filenames shouldn't be shortened and it doesn't need to include percentages or the total at the end.
Ping from triage:
@GuillaumeGomez can you address the comments above?
thanks.
There is nothing else for me to do except adding the full "reason". However, I'd prefer @QuietMisdreavus to provide it.
Status: Awaiting decision from the relevant subteam (see the T- label).
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
☔ The latest upstream changes (presumably #67532) made this pull request unmergeable. Please resolve the merge conflicts.
I updated the description. Will fix the conflicts soon as well.
| // FIXME(misdreavus): this needs to count graphemes, and probably also track |
|---|
| // double-wide characters... |
| if filename.len() > 35 { |
| "...".to_string() + &filename[filename.len() - 32..] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be what's causing #66472 (comment). Maybe you could use filename.char_indices().nth(32) instead?
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] |
|---|
| pub enum OutputFormat { |
| Json, |
| HTML, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] |
|---|
| pub enum OutputFormat { |
| Json, |
| HTML, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enum will need a third variant to represent the current output of --show-coverage. That way --show-coverage --output-format html will be an error.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to handle it outside instead.
| struct CoverageCalculator { |
|---|
| items: BTreeMap<FileName, ItemCount>, |
| output_format: Option, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output_format shouldn't be a field of CoverageCalculator, instead it should be passed as an argument to print_results.
| @@ -270,6 +270,7 @@ pub struct RenderInfo { |
|---|
| pub deref_trait_did: Option, |
| pub deref_mut_trait_did: Option, |
| pub owned_box_did: Option, |
| pub output_format: Option, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the output_format field would fit better on DocContext than RenderInfo.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's impacting the rendering so from my point of view, it fits better here. :)
| @@ -0,0 +1,5 @@ |
|---|
| // compile-flags:-Z unstable-options --output-format |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should be a rustdoc-ui test so we can check the error message.
| struct CoverageCalculatorEntry { |
|---|
| documented: u64, |
| total: u64, |
| percentage: f64, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason to include the percentage in the JSON. Whatever is consuming the JSON can trivially calculate it if needed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point.
| }) |
|---|
| .collect::<BTreeMap<String, CoverageCalculatorEntry>>(); |
| json.insert( |
| "total".to_owned(), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with the percentage, there's no reason to include the total in the JSON. It's just redundant information.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| } |
|---|
| #[derive(Serialize)] |
| struct CoverageCalculatorEntry { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the percentage removed this struct isn't needed, #[derive(Serialize)] can be added to ItemCount instead.
| CoverageCalculator { items: Default::default(), output_format } |
|---|
| } |
| fn to_json(&self) -> String { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cleaner to implement Serialize for CoverageCalculator directly.
| percentage: total.percentage().unwrap_or(0.0), |
|---|
| }, |
| ); |
| serde_json::to_string_pretty(&json).expect("failed to convert JSON data to string") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the JSON is to be consumed by some other tool then it doesn't need to be pretty printed.
@kinnison I rewrote the commit history to make it simpler to follow. Normally I already fixed all @ollie27's comments (unless I forgot one?).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is modifying --output-format we could do with rustdoc-ui tests generating normal docs with one passing --output-format html and one passing --output-format json.
Other than those issues with the tests this looks good.
| @@ -0,0 +1 @@ |
|---|
| {"$DIR/json.rs":{"total":13,"with_docs":7}} |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't yet pass on Windows because of how $DIR is normalized. Specifically
| if json { |
|---|
| from = from.replace("\\", "\\\\"); |
| } |
isn't being triggered but is needed in this case. This can be fixed by adding cflags.contains("--output-format json") || cflags.contains("--output-format=json") to
| let json = cflags.contains("--error-format json") |
|---|
| | |
| | |
| | |
.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch, thanks a lot!
| @@ -0,0 +1,4 @@ |
|---|
| // compile-flags:-Z unstable-options --output-format |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was this supposed to test? Was there meant to be something passed to --output-format? As this test is in the coverage directory, was --show-coverage supposed to be passed here?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's checking that it fails when no parameter is provided.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case it shouldn't be in the coverage directory and -Z unstable-options doesn't need to be passed. However more importantly, the error this test is actually producing is error: too many file operands because compiletest is adding more arguments after --output-format which are then getting misinterpreted by rustdoc. It may be best to just drop this test.
☔ The latest upstream changes (presumably #69592) made this pull request unmergeable. Please resolve the merge conflicts.
Removed the output-format test as well.
@ollie27 You had the majority of input on this PR -- have your raised concerns been dealt with sufficiently?
You had the majority of input on this PR -- have your raised concerns been dealt with sufficiently?
Yeah it looks like my main concerns have been addressed so r=me.
📌 Commit b646627 has been approved by ollie27
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
Status: Awaiting decision from the relevant subteam (see the T- label).
labels
⌛ Testing commit b646627 with merge 515b9890b404a710400bff9eb167caab4b0fd1d4...
bors added a commit that referenced this pull request
Rollup of 8 pull requests
Successful merges:
- #66472 (--show-coverage json)
- #69603 (tidy: replace
make checkwith./x.py testin documentation) - #69760 (Improve expression & attribute parsing)
- #69828 (fix memory leak when vec::IntoIter panics during drop)
- #69850 (panic_bounds_check: use caller_location, like PanicFnLangItem)
- #69876 (Add long error explanation for E0739)
- #69888 ([Miri] Use a session variable instead of checking for an env var always)
- #69893 (librustc_codegen_llvm: Use slices instead of 0-terminated strings)
Failed merges:
r? @ghost
☔ The latest upstream changes (presumably #69919) made this pull request unmergeable. Please resolve the merge conflicts.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
Labels
Status: This is awaiting some action (such as code changes or more information) from the author.