test: reach 100% coverage and enforce in CI#108
Merged
Conversation
Adds six targeted unit tests so coverage hits 100% (up from 99%): - env.py:191: FILTER_TEAMS with an empty middle entry (skip branch) - slack_notify.py:43: per-repo empty list when other repos are non-empty - pr_comment.py:92-98: new_conflict_keys lookup, both forward and reverse (PR-number, other) pair ordering - pr_comment.py:130, 139: failure paths for update and post returning False FILTER_TEAMS edge case is moved to a new TestEnvFilterTeamsEdgeCases class so TestEnv stays under pylint max-public-methods. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Zack Koppert <zkoppert@github.com>
Now that every line is exercised by tests, bump --cov-fail-under from 80 to 100 so a regression in coverage fails CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Zack Koppert <zkoppert@github.com>
Three independent code-review agents (Opus 4.7, Sonnet 4.6, GPT-5.4) flagged the same issues on the coverage tests added in the previous commit. Coverage of the lines was real, but the assertions did not verify the behavior those lines produce. - test_post_pr_comments_with_new_conflict_keys and test_post_pr_comments_new_conflict_keys_reverse_pair: inspect the body argument passed to _post_comment and assert the 🆕 badge appears. Without this, deleting pr_comment.py:91-98 would leave the tests green. - test_send_slack_notification_skips_repo_with_empty_list: wrap cluster_conflicts so the test can prove the 'continue' branch short-circuited. Without this, removing the continue would still produce one post (cluster_conflicts([]) returns []). - TestEnvFilterTeamsEdgeCases: fix the misleading docstring. The class exists to keep TestEnv under pylint's max-public-methods threshold, not because a test was moved out of TestEnv. All 287 tests still pass; coverage stays at 100%. Signed-off-by: Zack Koppert <zkoppert@github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Zack Koppert <zkoppert@github.com>
There was a problem hiding this comment.
Pull request overview
This PR strengthens the project’s quality gate by bringing Python test coverage from 99% to 100% with targeted unit tests, and then enforcing 100% coverage in CI via the make test target so future coverage regressions fail automatically.
Changes:
- Added unit tests to cover previously-missed branches in Slack notifications, PR comment posting (including failure paths), and
FILTER_TEAMSparsing edge cases. - Increased the pytest coverage enforcement threshold from 80% to 100% in the Makefile.
Show a summary per file
| File | Description |
|---|---|
test_env.py |
Adds an edge-case test ensuring empty comma-separated FILTER_TEAMS entries are skipped after whitespace stripping. |
test_pr_comment_posting.py |
Adds tests for 🆕 rendering via new_conflict_keys (both tuple orders) and for _post_comment/_update_comment failure returns. |
test_slack_notify.py |
Adds a mixed-repo test to exercise the per-repo skip branch when a repo’s conflict list is empty. |
Makefile |
Raises --cov-fail-under to 100 to enforce full coverage in CI via make test. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 0
jmeridth
approved these changes
Jun 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Proposed Changes
Test coverage was sitting at 99% with 8 uncovered lines spread across
env.py,pr_comment.py, andslack_notify.py. Each gap was a real branch that could regress silently, and there was no CI guardrail preventing coverage from drifting further down over time.I added six targeted unit tests so every line is now exercised, then bumped the pytest threshold so future regressions fail CI instead of being noticed later.
What changed
test_env.py- Added a FILTER_TEAMS case where a comma-separated value contains an empty middle entry ("a, , b"), confirming the parser skips it. The new case lives in a smallTestEnvFilterTeamsEdgeCasesclass soTestEnvstays under pylint'smax-public-methods.test_slack_notify.py- Added a case mixing a repo with zero conflicts and a repo with conflicts, so the per-repo skip branch is hit (the existing empty-conflict test took an earlier short-circuit).test_pr_comment_posting.py- Added four tests: forward and reverse(pr_number, other)lookups whennew_conflict_keysis supplied, plus failure-path coverage for both_update_commentand_post_commentreturning False.Makefile- Raised--cov-fail-underfrom 80 to 100 so any future drop in coverage fails CI.Impact
Readiness Checklist
Author/Contributor
make lintand fix any issues that you have introducedmake testand ensure you have test coverage for the lines you are introducingTesting
make testlocally: 287 passed, 100.00% coverage, threshold check at 100% passesterm-missingreport (zero missing lines on every module)TestEnvstays at 25 public methods, the newTestEnvFilterTeamsEdgeCasesclass isolates the new case so it does not regress thetoo-many-public-methodsruleRollout
No runtime behavior changes - the action's production code is untouched. The only behavioral change is in CI: a future PR that lowers coverage below 100% will fail
make test. If that becomes too strict for a specific change, the threshold in the Makefile is the single knob to tune.