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

fix ci: Set test coverage thresholds #84

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Conversation

yoomlam
Copy link
Contributor

@yoomlam yoomlam commented Oct 1, 2024

Ticket

Slack

Changes

Set test coverage thresholds.
The PR check will fail if thresholds aren't met.

Testing

Tested here: #83 (comment)
which now shows a red status.

Copy link

github-actions bot commented Oct 1, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
1826 1539 84% 80% 🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: 804869f by action🐍

Copy link
Contributor

@KevinJBoyer KevinJBoyer left a comment

Choose a reason for hiding this comment

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

LGTM with one thought that we might want to relax thresholdModified to 0 if I understand how it works correctly

@@ -56,3 +56,6 @@ jobs:
sourceDir: app
coverageFile: app/coverage_report.xml
token: ${{ secrets.GITHUB_TOKEN }}
thresholdAll: 0.8
thresholdNew: 0.9
thresholdModified: 0.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
thresholdModified: 0.8
thresholdModified: 0.8

Is this file specific -- in other words, does this mean edits to a file with no pre-existing coverage will fail this check if the PR doesn't take coverage from 0 -> 80%?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an exclusion for app.py:
2f13d8e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I'd recommend that files with no pre-existing coverage be added to the omit list rather than changing the threshold.

@yoomlam yoomlam merged commit 75bd1a1 into main Oct 1, 2024
10 checks passed
@yoomlam yoomlam deleted the yl/coverage_thresholds branch October 1, 2024 16:03
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