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

Improve coverage reports #361

Merged
merged 18 commits into from
Jul 11, 2024
Merged

Improve coverage reports #361

merged 18 commits into from
Jul 11, 2024

Conversation

2bndy5
Copy link
Collaborator

@2bndy5 2bndy5 commented Jul 4, 2024

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.

tests/sitemap_test.py Outdated Show resolved Hide resolved
@jbms
Copy link
Owner

jbms commented Jul 10, 2024

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
Copy link
Collaborator Author

2bndy5 commented Jul 10, 2024

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
Copy link
Owner

jbms commented Jul 10, 2024

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
Copy link

Welcome to Codecov 🎉

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 ☂️

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 11, 2024

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

@jbms
Copy link
Owner

jbms commented Jul 11, 2024

codecov

Not sure if there is a way to grant access.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 11, 2024

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
Copy link
Owner

jbms commented Jul 11, 2024

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
Copy link
Collaborator Author

2bndy5 commented Jul 11, 2024

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

@2bndy5 2bndy5 merged commit 1aa0123 into main Jul 11, 2024
70 checks passed
@2bndy5 2bndy5 deleted the improve-coverage branch July 11, 2024 06:27
2bndy5 added a commit that referenced this pull request Jul 11, 2024
2bndy5 added a commit that referenced this pull request Jul 11, 2024
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