aantron / bisect_ppx

Code coverage for OCaml and ReScript
http://aantron.github.io/bisect_ppx/demo/
MIT License
301 stars 60 forks source link

RFC: fancier index with per-directory summaries and collapsible directory tree #396

Closed arvidj closed 2 years ago

arvidj commented 2 years ago

This MR makes the index.html page in the generated documentation easier to read for larger projects:

Screenshot from 2022-02-03 13-22-08

It adds:

  1. points stats to each line (makes it easier to deduce the size of a file)
  2. per-directory summary, with the average coverage of the directory and the (visited / total) points
  3. collapsible sections per directory

I've built these three things on top of each other, so it'd be quite easy to cherry-pick parts of the proposal.

As it is, the MR is in a little bit of a mess, I'd like to check first if there is interest in such a proposal. Notably:

The last commit contains an example project (as in the image above), and a script for generating the report.

If you like it, I'll start cleaning up.

aantron commented 2 years ago

Thanks. It looks like a good improvement!

I'd request two additional things:

@ejgallego If you have time, could you try this on Coq? It's the only other really large project I know of that is using/considering Bisect_ppx (https://github.com/coq/coq/pull/12259).

arvidj commented 2 years ago

Hi Anton, thanks for your thoughtful comments!

Hiding the nested tree part of this behind an option, like bisect-ppx-report html --tree or similar. For small projects, the tree adds noise/confusion, especially if they have only one source directory. So, I think the PR should add the per-file statistics to all output, but format the output as a tree only for large projects, when asked to. If you'd like, I can help out with this.

Sure! I was thinking about it, and you could actually implement it as a theme. Then the markup would always be "tree-compatible" but depending on the theme you choose, you either see the "classic reports" or you get a report with collapsible directories. The disadvantage is that you would have to have 4 themes if you want (light / dark + classic / tree). The advantage is that there is no need to adapt the markup. On the other hand, programming in OCaml is nicer than programming in CSS.

Adapting the CSS to the dark theme (you can force generation with the dark theme with bisect-ppx-report html --theme dark, or set your browser's theme to dark).

Sure!

aantron commented 2 years ago

On the other hand, programming in OCaml is nicer than programming in CSS.

Indeed. It's probably best to actually emit different markup rather than do this with themes. Doing it with themes might get more and more difficult to maintain if the number of themes ever starts growing.

arvidj commented 2 years ago

Here's an example of a larger report: https://arvidj.eu/_tezos_coverage.2022-02-04/

aantron commented 2 years ago

Here's an example of a larger report: https://arvidj.eu/_tezos_coverage.2022-02-04/

Thanks. It's pretty clear that this PR's kind of feature is useful :)

arvidj commented 2 years ago

I've added the tree option and I've made an adaption for the dark theme.

What remains to do:

aantron commented 2 years ago
  • clean up the history. any specific thoughts about this or do you think it could pass as a single commit PR?

No need to do this. I will squash this PR to one commit.

arvidj commented 2 years ago

Okay, I think this is ready for review now. I've fixed the existing test (I only had to modify one cram test) and I added a cram test for the html-tree.

For the open/close buttons, I've added two easter eggs:

aantron commented 2 years ago

Снимок

Could you please fix the numbers so that there aren't underscores in the middle?

arvidj commented 2 years ago

Снимок

Could you please fix the numbers so that there aren't underscores in the middle?

For sure, just did that. The reason I added it was to make large numbers a bit more readable, at the request of a pointy-haired boss hehe. An alternative would to use:

I'd prefer spaces in this case but I don't feel strongly about this (or the need to have any digit grouping whatsoever).

aantron commented 2 years ago

Part of the issue with these separators is they all either look strange (non-standard), or conflict with the various standard but local conventions for decimal and thousands delimiters, which differ from country to country. Given that Bisect is rarely run on files with thousands of instrumentation points, and even more rarely tens of thousands, but probably never millions, I think it's best to just not use separators. We can come back to this later, though, upon more feedback :)

aantron commented 2 years ago

Thank you for this PR!