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

Evaluate adding diff-cover to automatically require added/changed lines to be covered #89

Open
lsorber opened this issue Aug 8, 2022 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@lsorber
Copy link
Member

lsorber commented Aug 8, 2022

Considerations:

  1. Is requiring 100% coverage on added/changed lines too strict? Maybe we should go for 50% by default and leave it up to the user to set it higher if they wish.
  2. The tool works by comparing against a remote branch like origin/main (the default), which needs to be configured in pyproject.toml. What happens is that branch doesn't exist? Is there a way we can figure this out from Cookiecutter?

https://github.com/Bachmann1234/diff_cover

@lsorber lsorber self-assigned this Aug 8, 2022
@lsorber lsorber added the enhancement New feature or request label Aug 8, 2022
@sinopeus
Copy link
Collaborator

sinopeus commented Feb 23, 2023

Regarding 1, 50% is a sane default, 100% is unrealistic and optimizes for the wrong targets. A possible solution for point 2 is using a variation on this shell script at the end of the poe test task:

export COMPARE_BRANCH=${CI_MERGE_REQUEST_TARGET_BRANCH_NAME:-$(git rev-parse --abbrev-ref HEAD)}
git fetch origin ${COMPARE_BRANCH}:refs/remotes/origin/${COMPARE_BRANCH}
diff-cover --ignore-unstaged --compare-branch $COMPARE_BRANCH -c pyproject.toml reports/coverage.xml

This only works for GitLab CI/CD, but I assume that GitHub Actions exposes a similar environment variable. In the case of an MR or PR, this compares against the target branch, which is what we want. In other cases it compares against the current branch, which in practice means you compute the coverage of the changes staged in Git (similar to how pre-commit ignores unstaged changes by default).

@lsorber
Copy link
Member Author

lsorber commented Feb 23, 2023

We could decide to accept the default of main, as it's the default branch name for both GitHub and GitLab now so new projects are likely to be using it. And those that use a different default branch name can always configure it themselves.

Alternatively, maybe would use something like

COMPARE_BRANCH=$(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "main")

to avoid depending on CI environment variables.

@sinopeus
Copy link
Collaborator

It's a sane default but doesn't cover cases where you can have MRs targeted at other branches than the main branch, e.g. Git Flow, where you typically merge from feature branches into develop or from release branches into main. But a simple, opinionated option like what you propose is great as well 🙂

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

No branches or pull requests

2 participants