Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add covrpage vignette to communicate package testing health #211

Closed
wants to merge 0 commits into from

Conversation

yonicd
Copy link
Collaborator

@yonicd yonicd commented Dec 23, 2022

added covrpage output to the package vignettes/site and tests/readme.md to help to communicate package health to prospective users.

gha also added to allow for autogeneration of readme on commit/PR

@danielinteractive
Copy link
Collaborator

Thanks @yonicd !
@cicdguy cqn you please review re: automation?

@danielinteractive danielinteractive self-assigned this Jan 3, 2023
Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @yonicd , nice page! see one question inside. Also:

  • Would it also be possible to let the reader click on a file name e.g. and then see highlighted which lines of code are not covered by tests?
  • I would like to let @cicdguy review the github workflows
  • you can add yourself to contributors in the package description

git config --local user.email "[email protected]"
git config --local user.name "GitHub Actions"
Rscript -e 'remotes::install_github("hadley/emo")' \
-e 'remotes::install_github("yonicd/covrpage@actions")' \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need that specific branch? Did you already try to make a PR towards the main covrpage repo? just thinking for longer term maintenance that would be better.

Copy link
Collaborator Author

@yonicd yonicd Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The links for the tests are taking you directly to the lines that were not run. There are icons next to tests that gave either warnings/failures as a way to know which ones were the offending ones.

I merged that branch into main, so that can be amended to be yonicd/covrpage.

@cicdguy
Copy link
Contributor

cicdguy commented Jan 3, 2023

Hi @yonicd, thank you for adding this.

I'd prefer to simply upload the covr HTML report to our pkgdown website instead of using the Rmd-based approach you're taking, and also introducing a new workflow is operational (and compute) overhead.

@@ -13,6 +13,7 @@ on:
jobs:
docs:
name: Pkgdown Docs 📚
if: "!contains(github.event.head_commit.message, 'skip docs')"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yonicd
Copy link
Collaborator Author

yonicd commented Jan 3, 2023

This tool is meant as a communication to general users the testing diversity and coverage in a package and have it accessible on the github level (can't read html in github pages). The covr html output leaves a lot of information out of the summary report which is less useful for non-programmers to digest.

@cicdguy
Copy link
Contributor

cicdguy commented Jan 3, 2023

This tool is meant as a communication to general users the testing diversity and coverage in a package and have it accessible on the github level (can't read html in github pages). The covr html output leaves a lot of information out of the summary report which is less useful for non-programmers to digest.

You mean like we already do on PRs like here and here? We also upload all info directly to the workflow summary, see example.

@yonicd
Copy link
Collaborator Author

yonicd commented Jan 3, 2023

those are great. but to @danielinteractive's question above. covrpage gives you links to each test and it will run even if test(s) fail so you get the coverage regardless and you can link directly to what was the problem area. Users of covrpage have found that having the report tests/readme.md makes the package more accessible to prospective users and helps with developmemt collaborations.

@cicdguy
Copy link
Contributor

cicdguy commented Jan 3, 2023

@danielinteractive I'm okay with this as long as you're okay with this. I wouldn't take responsibility of maintaining this in the long run, however.

@danielinteractive
Copy link
Collaborator

Thanks @cicdguy and @yonicd - I am open to this change but I wonder a bit if we really need this and if we can only have one coverage run instead of two for each commit.

In this package coverage is so high that the coverage badge which I think we have basically says all for users?

For developers on the other hand the coverage report that is already generated as part of GHA is more relevant as it shows during PR what is missing.

@cicdguy
Copy link
Contributor

cicdguy commented Jan 3, 2023

Architecturally, and from a technical optimization perspective, it makes sense to

  1. Upload the HTML reports from covr directly into GitHub pages
  2. Create a pkgdown navbar menu item to point to the HTML reports
  3. Update the badge link so that it points to the covr HTML report instead of the XML report like it does currently

That way we don't need to run 2 coverage workflows, maintain an Rmd and a README, and therefore reduce package size (by removing the README.md and the vignette).

But again, totally up to you to decide how you want to proceed @danielinteractive

@danielinteractive
Copy link
Collaborator

Sounds like a good idea to me - @yonicd what do you think?

@yonicd
Copy link
Collaborator Author

yonicd commented Jan 3, 2023

It is up to you guys.

  • We will be talking about the transparency and application of tests in other content in the working group. Part of the planned discussion will be different strategies of communication, and the comparison of covr and covrpage will be added there.
  • wrt to package size, I don't think that 2 small text files will move the needle.
  • Part of my motivation of covrpage was that the covr badge is a poor way to communicate package health. It is a crude summary statistic that can mask a lot of problems.
  • Report information is comparable, the only differences are that you do not get a report if the package fails a test and there are no links to specific lines for tests in covr.

@danielinteractive
Copy link
Collaborator

Thanks @yonicd!
I think the covr and covrpage options are both nice and worth discussing and introducing as part of the SWE WG.
Since in this package and repo we already use covr I would tend to keep using that route for this package.

How about @cicdguy would you create a separate PR with the proposed changes and then we can compare more easily, regarding output and also regarding extra changes in the code? I guess that PR could be nice to include then also in the R package template repo so other projects can benefit.

@danielinteractive
Copy link
Collaborator

@cicdguy any chance we can get the alternative PR soon?

@cicdguy
Copy link
Contributor

cicdguy commented Feb 24, 2023

There's a POC being done by @walkowif here: https://walkowif.github.io/tern/main/
This will come to mmrm following a few more UX-centric iterations. Stay tuned.

@danielinteractive
Copy link
Collaborator

Great thanks @cicdguy for the good news :-) Looking forward! Let me know when I can help testing or so.

@yonicd
Copy link
Collaborator Author

yonicd commented Feb 26, 2023

looks interesting.

A few questions

coverage:

  • The coverage page is the html output the covr package gives? how large is that output (package size-wise) and additional js deps to support the tabs (datatables, crosstalk, highlight...)?

unit testing page:

  • Is there any way to present the unit testing in a more inviting format to regular users trying to understand what has been tested. The current format looks like a long log file that has been dumped into html format, I am not sure if that serves the intended purpose of communicating to general users.

@cicdguy
Copy link
Contributor

cicdguy commented Feb 26, 2023

  • The coverage page is the html output the covr package gives? how large is that output (package size-wise) and additional js deps to support the tabs (datatables, crosstalk, highlight...)?

The size doesn't really matter as the docs will be stored outside of the R package itself. Yes there are additional assets that covr uses (ideally they should be using a CDN but 🤷) but those go hand-in-hand with all of the other assets that pkgdown itself uses (again, no CDN used there either so you'll always end up with bundled assets 🤷).

  • Is there any way to present the unit testing in a more inviting format to regular users trying to understand what has been tested. The current format looks like a long log file that has been dumped into html format, I am not sure if that serves the intended purpose of communicating to general users.

We're using https://github.com/lukejpreston/xunit-viewer for creating the HTML reports. If test suites have human-readable names and so do the test cases, the report can be more intuitive.

There's also https://gitlab.com/inorton/junit2html as an alternative that can be used to report the overall test report. See example here: https://inorton.gitlab.io/-/junit2html/-/jobs/3809671223/artifacts/junit2html-merged-example.xml.html, but frankly the UI doesn't seem very appealing. The former gives us the option to do some additional branding as well.

We also have CI-based reporting to show test performance. See example here: insightsengineering/tern#838 (comment), which we'll add to mmrm soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants