jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
177 stars 28 forks source link

Improve coverage reports #361

Closed 2bndy5 closed 3 days ago

2bndy5 commented 1 week ago

This adds unit tests for some of our simpler extensions. The apidoc.* and external_resources extensions still need some tests...

I mostly just used examples from the docs to create the new tests. This is a better measurement because unit tests are done in isolation, no other extensions complicate expected behavior.

I'm seeing a 7% increase in coverage in CI (compared to current main branch).

~As a continued avoidance of third-party services (like CodeCov), I also adjusted the CI workflow. Now, the coverage report's HTML output is uploaded to the gh-pages branch (in addition to Linux-built docs) upon push to main branch. Also, I added an entry to our docs' version selector for easy access to the latest coverage reports. This will help us see exactly which lines were invoked during unit tests (on main branch) without using nox locally.~

The pytest output will now show the top 5 longest running tests.

jbms commented 4 days ago

Thanks, this is great.

The one thing I'm not so sure about is how to access the coverage report.

Accessing it via the version selector has the advantage of putting the version selector to use, but on the other hand may be a bit confusing since it is kind of a non-standard usage of that feature.

Additionally, it seems to me that someone browsing the docs is unlikely to want to visit the coverage report --- since the coverage is only interesting during development. Furthermore, I imagine it is most useful to access the coverage for an open PR, but as far as I understand, that is not currently possible with what you have here?

Unfortunately github actions does not provide a way to directly browse artifacts --- which is why we are relying on readthedocs for previews on PRs.

Regarding reliance on third-party services: for hosting the docs themselves, I was disinclined to use readthedocs as our primary hosting method since it includes ads. For a coverage report that is used only during development I don't have any problem relying on a third-party service, though.

2bndy5 commented 4 days ago

For a coverage report that is used only during development I don't have any problem relying on a third-party service, though.

I'm glad to hear that. I don't have the access to setup a CODECOV_TOKEN in the repo -> settings -> "secrets and variables" -> dependabot/actions. Note, dependabot has a special set of secrets apart from GitHub actions. For a PR from dependabot to submit coverage reports to codecov, the token needs to be a duplicate secret (using the same variable name) for both settings menus.

We don't have to use CodeCov exactly. Its just the most common among open source, and I'm very familiar with it's configuration. It allows logins using GitHub accounts.

Once the token is setup, I can make the necessary changes here.

PS - I'm also about to set this up for a rust-based embedded hardware driver project that I just started...

jbms commented 4 days ago

I added the CODECOV tokens including for dependabot.

However, regarding dependabot, presumably the same issue of lacking permission to upload coverage reports would also apply to anyone other than you or I that is submitting a PR -- granted that is rare.

Github has a workflow_run action type that can be used to workaround this limitation --- I've used that in a different project to deploy a preview using credentials:

https://github.com/google/neuroglancer/blob/master/.github/workflows/deploy_preview.yml

Essentially the idea is that the normal PR workflow just builds the coverage report and uploads it as an artifact. Then a separate workflow run from the master branch of the repo (not the PR) is triggered by the completion of the normal PR workflow, downloads the artifact, and uploads the coverage report.

codecov-commenter commented 4 days ago

Welcome to Codecov :tada:

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

2bndy5 commented 4 days ago

I would also add a codecov badge to the README, but I don't have the appropriate access to the associated codecov project.

jbms commented 3 days ago

codecov

Not sure if there is a way to grant access.

2bndy5 commented 3 days ago

Not sure if there is a way to grant access.

There would be inherent access granted to the org that owns the repo. I do this with nRF24 and cpp-linter orgs on github. But this repo does not apply to such shared accessibility on codecov (might be a 💰-guarded feature if any). No worries, though.

I added the badge to the README, thanks.

jbms commented 3 days ago

This is a good start -- these test that various features build without warnings/errors but snapshot tests could be helpful for confirming that the output is as expected. On the other hand those are likely to be brittle and dependent on docutils and sphinx version, which may mean they are more trouble than they are worth.

2bndy5 commented 3 days ago

Yeah, the "brittle" argument is why the breathe project discounted the idea as well (based on experience from breathe's original author).