--show-coverage json #66472

GuillaumeGomez · 2019-11-16T15:23:58Z

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.

r? @QuietMisdreavus

src/librustdoc/passes/calculate_doc_coverage.rs

GuillaumeGomez · 2019-11-17T13:33:34Z

Updated suggestions.

bors · 2019-11-18T22:10:51Z

☔ The latest upstream changes (presumably #54733) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2019-11-21T12:20:56Z

cc @Centril

Centril · 2019-11-21T12:22:55Z

I don't have enough experience with rustdoc to review this, sorry :)

GuillaumeGomez · 2019-11-21T13:16:05Z

Ah my bad, since you commented I thought you felt up for this. ;)

r? @kinnison

ollie27 · 2019-11-21T17:49:46Z

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?

GuillaumeGomez · 2019-11-21T21:04:52Z

@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.

GuillaumeGomez · 2019-11-21T21:05:07Z

Oh also; it's a request coming from @QuietMisdreavus. :)

kinnison · 2019-11-22T21:03:21Z

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?

ollie27 · 2019-11-22T23:02:29Z

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.

JohnCSimon · 2019-11-30T05:25:45Z

Ping from triage:
@GuillaumeGomez can you address the comments above?
thanks.

GuillaumeGomez · 2019-11-30T10:46:32Z

There is nothing else for me to do except adding the full "reason". However, I'd prefer @QuietMisdreavus to provide it.

bors · 2019-12-22T22:40:14Z

☔ The latest upstream changes (presumably #67532) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2020-01-01T13:30:08Z

I updated the description. Will fix the conflicts soon as well.

jyn514 · 2020-02-12T22:37:31Z

src/librustdoc/passes/calculate_doc_coverage.rs

+    // FIXME(misdreavus): this needs to count graphemes, and probably also track
+    // double-wide characters...
+    if filename.len() > 35 {
+        "...".to_string() + &filename[filename.len() - 32..]


I think this might be what's causing #66472 (comment). Maybe you could use filename.char_indices().nth(32) instead?

GuillaumeGomez · 2020-02-17T13:10:40Z

Updated!

ollie27 · 2020-02-17T16:41:58Z

src/librustdoc/config.rs

+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+pub enum OutputFormat {
+    Json,
+    HTML,


Suggested change

HTML,

Html,

ollie27 · 2020-02-17T16:44:09Z

src/librustdoc/config.rs

+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+pub enum OutputFormat {
+    Json,
+    HTML,


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.

I prefer to handle it outside instead.

ollie27 · 2020-02-17T16:47:33Z

src/librustdoc/passes/calculate_doc_coverage.rs

 struct CoverageCalculator {
    items: BTreeMap<FileName, ItemCount>,
+    output_format: Option<OutputFormat>,


output_format shouldn't be a field of CoverageCalculator, instead it should be passed as an argument to print_results.

ollie27 · 2020-02-17T17:07:13Z

src/librustdoc/html/render.rs

@@ -270,6 +270,7 @@ pub struct RenderInfo {
    pub deref_trait_did: Option<DefId>,
    pub deref_mut_trait_did: Option<DefId>,
    pub owned_box_did: Option<DefId>,
+    pub output_format: Option<OutputFormat>,


I think the output_format field would fit better on DocContext than RenderInfo.

It's impacting the rendering so from my point of view, it fits better here. :)

ollie27 · 2020-02-17T17:07:50Z

src/test/rustdoc/output-format-fail-check.rs

@@ -0,0 +1,5 @@
+// compile-flags:-Z unstable-options --output-format


This test should be a rustdoc-ui test so we can check the error message.

ollie27 · 2020-02-17T17:12:07Z

src/librustdoc/passes/calculate_doc_coverage.rs

+struct CoverageCalculatorEntry {
+    documented: u64,
+    total: u64,
+    percentage: f64,


There's no reason to include the percentage in the JSON. Whatever is consuming the JSON can trivially calculate it if needed.

This is a good point.

ollie27 · 2020-02-17T17:13:11Z

src/librustdoc/passes/calculate_doc_coverage.rs

+            })
+            .collect::<BTreeMap<String, CoverageCalculatorEntry>>();
+        json.insert(
+            "total".to_owned(),


As with the percentage, there's no reason to include the total in the JSON. It's just redundant information.

ollie27 · 2020-02-17T17:14:43Z

src/librustdoc/passes/calculate_doc_coverage.rs

+}
+
+#[derive(Serialize)]
+struct CoverageCalculatorEntry {


With the percentage removed this struct isn't needed, #[derive(Serialize)] can be added to ItemCount instead.

ollie27 · 2020-02-17T17:16:09Z

src/librustdoc/passes/calculate_doc_coverage.rs

+        CoverageCalculator { items: Default::default(), output_format }
+    }
+
+    fn to_json(&self) -> String {


It would be cleaner to implement Serialize for CoverageCalculator directly.

ollie27 · 2020-02-17T17:17:52Z

src/librustdoc/passes/calculate_doc_coverage.rs

+                percentage: total.percentage().unwrap_or(0.0),
+            },
+        );
+        serde_json::to_string_pretty(&json).expect("failed to convert JSON data to string")


If the JSON is to be consumed by some other tool then it doesn't need to be pretty printed.

GuillaumeGomez · 2020-02-21T12:46:00Z

@kinnison I rewrote the commit history to make it simpler to follow. Normally I already fixed all @ollie27's comments (unless I forgot one?).

ollie27

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.

ollie27 · 2020-02-27T00:12:13Z

src/test/rustdoc-ui/coverage/json.stdout

@@ -0,0 +1 @@
+{"$DIR/json.rs":{"total":13,"with_docs":7}}


This test doesn't yet pass on Windows because of how $DIR is normalized. Specifically

rust/src/tools/compiletest/src/runtest.rs

Lines 3213 to 3215 in abc3073

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

rust/src/tools/compiletest/src/runtest.rs

Lines 3204 to 3207 in abc3073

let json = cflags.contains("--error-format json")

|| cflags.contains("--error-format pretty-json")

|| cflags.contains("--error-format=json")

|| cflags.contains("--error-format=pretty-json");

.

Great catch, thanks a lot!

ollie27 · 2020-02-27T00:12:40Z

src/test/rustdoc-ui/coverage/output-format-fail-check.rs

@@ -0,0 +1,4 @@
+// compile-flags:-Z unstable-options --output-format


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?

It's checking that it fails when no parameter is provided.

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.

bors · 2020-03-01T04:43:16Z

☔ The latest upstream changes (presumably #69592) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2020-03-02T12:21:11Z

Removed the output-format test as well.

kinnison · 2020-03-08T09:35:43Z

@ollie27 You had the majority of input on this PR -- have your raised concerns been dealt with sufficiently?

ollie27 · 2020-03-10T02:05:11Z

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.

kinnison · 2020-03-10T19:40:35Z

@bors r=ollie27

bors · 2020-03-10T19:40:36Z

📌 Commit b646627 has been approved by ollie27

bors · 2020-03-10T19:40:37Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

bors · 2020-03-11T12:51:07Z

⌛ Testing commit b646627 with merge 515b9890b404a710400bff9eb167caab4b0fd1d4...

Centril · 2020-03-11T13:05:46Z

@bors retry

@ghost

Rollup of 8 pull requests Successful merges: - #66472 (--show-coverage json) - #69603 (tidy: replace `make check` with `./x.py test` in 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

bors · 2020-03-11T16:17:28Z

⌛ Testing commit b646627 with merge c20d7ee...

bors · 2020-03-11T16:18:06Z

☔ The latest upstream changes (presumably #69919) made this pull request unmergeable. Please resolve the merge conflicts.

rust-highfive assigned QuietMisdreavus Nov 16, 2019

JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2019

Centril reviewed Nov 16, 2019

View reviewed changes

src/librustdoc/passes/calculate_doc_coverage.rs Outdated Show resolved Hide resolved

src/librustdoc/passes/calculate_doc_coverage.rs Outdated Show resolved Hide resolved

src/librustdoc/passes/calculate_doc_coverage.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the show-coverage-json branch from cf05f04 to 1de0f21 Compare November 17, 2019 12:49

GuillaumeGomez force-pushed the show-coverage-json branch from 1de0f21 to ea80202 Compare November 21, 2019 12:19

rust-highfive assigned kinnison and unassigned QuietMisdreavus Nov 21, 2019

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2019

Dylan-DPC-zz added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 1, 2019

jyn514 reviewed Feb 12, 2020

View reviewed changes

GuillaumeGomez force-pushed the show-coverage-json branch from ea80202 to 3c59767 Compare February 17, 2020 12:53

ollie27 suggested changes Feb 17, 2020

View reviewed changes

GuillaumeGomez force-pushed the show-coverage-json branch from d54ddd3 to d4c6522 Compare February 18, 2020 13:03

ollie27 reviewed Feb 27, 2020

View reviewed changes

GuillaumeGomez added 6 commits March 2, 2020 13:15

Put back output-format option for show-coverage

8858d71

Add support for json output in show-coverage option

10492c3

add tests for rustdoc output-format json

15babed

Replace ToJson with serde

f1070b1

Extend json detection in runtest

1c01646

remove output-format test

b646627

GuillaumeGomez force-pushed the show-coverage-json branch from a0ea237 to b646627 Compare March 2, 2020 12:20

Centril mentioned this pull request Mar 11, 2020

Rollup of 8 pull requests #69919

Merged

bors added S-waiting-on-author 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 Mar 11, 2020

bors merged commit 741d4ff into rust-lang:master Mar 11, 2020

GuillaumeGomez deleted the show-coverage-json branch March 12, 2020 10:37

		@@ -0,0 +1,5 @@
		// compile-flags:-Z unstable-options --output-format

	let json = cflags.contains("--error-format json")
	\|\| cflags.contains("--error-format pretty-json")
	\|\| cflags.contains("--error-format=json")
	\|\| cflags.contains("--error-format=pretty-json");

		@@ -0,0 +1,4 @@
		// compile-flags:-Z unstable-options --output-format

		@@ -0,0 +1 @@
		{"$DIR/json.rs":{"total":13,"with_docs":7}}

--show-coverage json #66472

--show-coverage json #66472

Conversation

GuillaumeGomez commented Nov 16, 2019 • edited Loading

GuillaumeGomez commented Nov 17, 2019

bors commented Nov 18, 2019

GuillaumeGomez commented Nov 21, 2019

Centril commented Nov 21, 2019

GuillaumeGomez commented Nov 21, 2019

ollie27 commented Nov 21, 2019

GuillaumeGomez commented Nov 21, 2019

GuillaumeGomez commented Nov 21, 2019

kinnison commented Nov 22, 2019

ollie27 commented Nov 22, 2019

JohnCSimon commented Nov 30, 2019

GuillaumeGomez commented Nov 30, 2019

bors commented Dec 22, 2019

GuillaumeGomez commented Jan 1, 2020

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 21, 2020

ollie27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 1, 2020

GuillaumeGomez commented Mar 2, 2020

kinnison commented Mar 8, 2020

ollie27 commented Mar 10, 2020

kinnison commented Mar 10, 2020

bors commented Mar 10, 2020

bors commented Mar 10, 2020

bors commented Mar 11, 2020

Centril commented Mar 11, 2020

bors commented Mar 11, 2020

bors commented Mar 11, 2020

GuillaumeGomez commented Nov 16, 2019 •

edited

Loading