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.

Sign up for GitHub

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

Jump to bottom

Covbadge #370

Closed
wants to merge 4 commits into from
Closed

Covbadge #370

wants to merge 4 commits into from

Conversation

stefsmeets
Copy link
Member

@stefsmeets stefsmeets commented Jan 8, 2024 •
edited
Loading

Description

This PR adds a workflow based on coverage.py and gists to generate a coverage badge. The advantage of this is that no third party service is needed.

Closes #369

Instructions to review the pull request

Create a python-template-test repo on GitHub (will be overwritten if existing)

cd $(mktemp -d --tmpdir py-tmpl-XXXXXX)
cookiecutter -c <pr-branch> https://github.com/<pr-user>/python-template
# Fill with python-template-test info
cd python-template-test
git init
git add --all
git commit -m "First commit"
git remote add origin https://github.com/<you>/python-template-test
git push -u origin main -f
python -m venv env
source env/bin/activate
python -m pip install --upgrade pip setuptools
python -m pip install '.[dev,publishing]'

egpbos
egpbos reviewed Jan 10, 2024
View reviewed changes
Copy link
Member

@egpbos egpbos left a comment

Choose a reason for hiding this comment

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

Looking good so far. Not sure why the tests are failing... Seems to be some Jinja error, see my comment for a possible culprit.

A possible solution would be to do a separate workflow for coverage. For that workflow, you can just run for one Python version (the newest, probably). The other build jobs can then just run without coverage, like before. I don't think this setup is currently integrating coverage reports from different Python versions anyway, right? So they are redundant.

{{cookiecutter.directory_name}}/README.md
@@ -11,13 +11,12 @@
| (5/5) checklist | [![workflow cii badge](https://bestpractices.coreinfrastructure.org/projects/<replace-with-created-project-identifier>/badge)](https://bestpractices.coreinfrastructure.org/projects/<replace-with-created-project-identifier>) |
| howfairis | [![fair-software badge](https://img.shields.io/badge/fair--software.eu-%E2%97%8F%20%20%E2%97%8F%20%20%E2%97%8F%20%20%E2%97%8F%20%20%E2%97%8B-yellow)](https://fair-software.eu) |
| **Other best practices** | &nbsp; |
| Static analysis | [![workflow scq badge](https://sonarcloud.io/api/project_badges/measure?project={{cookiecutter.github_organization}}_{{cookiecutter.directory_name}}&metric=alert_status)](https://sonarcloud.io/dashboard?id={{cookiecutter.github_organization}}_{{cookiecutter.directory_name}}) |
| Coverage | [![workflow scc badge](https://sonarcloud.io/api/project_badges/measure?project={{cookiecutter.github_organization}}_{{cookiecutter.directory_name}}&metric=coverage)](https://sonarcloud.io/dashboard?id={{cookiecutter.github_organization}}_{{cookiecutter.directory_name}}) |
<!-- TODO -->
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make an issue for this. We could also keep SonarCloud for static analysis, but maybe you or others have other ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Or is the TODO a message for the user of the template to change the Coverage badge URL?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's more for myself, the user would still need to put their gist id here. I'm not sure yet how to best do that the cookiecutter way.

{{cookiecutter.directory_name}}/.github/workflows/build.yml
coverage report >> $GITHUB_STEP_SUMMARY
echo $'\n```' >> $GITHUB_STEP_SUMMARY
- name: Make coverage badge
if: ${{ github.ref == 'refs/heads/main' && matrix.python-version == '3.12' && matrix.os == 'ubuntu-latest' }}
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this is what's tripping up Jinja in the failing tests. Try taking the &&s out. It probably recognizes this as a Jinja expression and cannot deal with &s apparently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that makes sense. I didn't immediately recognize these could be jinja2 templates.

@egpbos
Copy link
Member

egpbos commented Jan 10, 2024

Thanks for taking the time for this!

Before putting more effort in, though, let's finish the discussion in #369 that @bouweandela started.

@stefsmeets stefsmeets mentioned this pull request Jan 10, 2024
Open
@stefsmeets stefsmeets closed this Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egpbos egpbos egpbos left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

move from SonarCloud to reporting code quality and coverage through GitHub Actions
2 participants
@stefsmeets @egpbos