Skip to content

Commit 9070224

Browse files
zkoppertCopilot
zkoppert
and
Copilot
authored
test: reach 100% coverage and enforce in CI (#108)
* test: cover remaining uncovered branches to reach 100% 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> * ci: raise pytest coverage threshold to 100% 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> * test: tighten new assertions after multi-model review 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> --------- Signed-off-by: Zack Koppert <zkoppert@github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent b768a5a commit 9070224

4 files changed

Lines changed: 128 additions & 1 deletion

File tree

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
.PHONY: test
22
test:
3-
uv run python -m pytest -v --cov=. --cov-config=.coveragerc --cov-fail-under=80 --cov-report term-missing
3+
uv run python -m pytest -v --cov=. --cov-config=.coveragerc --cov-fail-under=100 --cov-report term-missing
44

55
.PHONY: clean
66
clean:

‎test_env.py‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -462,5 +462,32 @@ def test_get_env_vars_exclude_authors_defaults_to_empty(self):
462462
self.assertEqual(result.exclude_authors, [])
463463

464464

465+
class TestEnvFilterTeamsEdgeCases(unittest.TestCase):
466+
"""Edge cases for FILTER_TEAMS parsing.
467+
468+
Lives in a sibling class (rather than TestEnv) so TestEnv stays under
469+
pylint's max-public-methods threshold.
470+
"""
471+
472+
def setUp(self):
473+
for key in ("ORGANIZATION", "GH_TOKEN", "FILTER_TEAMS"):
474+
if key in os.environ:
475+
del os.environ[key]
476+
477+
@patch.dict(
478+
os.environ,
479+
{
480+
"ORGANIZATION": "my_organization",
481+
"GH_TOKEN": "my_token",
482+
"FILTER_TEAMS": "my-org/team-a, , my-org/team-b",
483+
},
484+
clear=True,
485+
)
486+
def test_filter_teams_skips_empty_comma_separated_entries(self):
487+
"""Empty entries between commas (after stripping whitespace) should be skipped."""
488+
result = get_env_vars(True)
489+
self.assertEqual(result.filter_teams, ["my-org/team-a", "my-org/team-b"])
490+
491+
465492
if __name__ == "__main__":
466493
unittest.main()

‎test_pr_comment_posting.py‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,82 @@ def test_multiple_conflicts_single_comment(self, _mock_find, mock_post):
104104
self.assertIn("#3", body)
105105
self.assertIn("**2**", body)
106106

107+
@patch("pr_comment._post_comment", return_value=True)
108+
@patch("pr_comment._find_existing_comments", return_value=[])
109+
def test_post_pr_comments_with_new_conflict_keys(self, _mock_find, mock_post):
110+
"""Newly detected conflicts should render with the 🆕 badge via new_conflict_keys."""
111+
conflict = _make_comment_conflict()
112+
conflicts = {"org/repo": [conflict]}
113+
114+
gh = MagicMock()
115+
gh.repository.return_value = MagicMock()
116+
117+
new_keys = {(1, 2)}
118+
result = pr_comment.post_pr_comments(conflicts, gh, new_conflict_keys=new_keys)
119+
120+
self.assertTrue(result)
121+
self.assertEqual(mock_post.call_count, 2)
122+
123+
# Each comment body should contain the 🆕 badge next to the OTHER PR.
124+
# _post_comment(repo, pr_number, body) — pr_number=positional[1], body=positional[2].
125+
bodies_by_pr = {call.args[1]: call.args[2] for call in mock_post.call_args_list}
126+
self.assertIn("🆕", bodies_by_pr[1])
127+
self.assertIn("🆕", bodies_by_pr[2])
128+
129+
@patch("pr_comment._post_comment", return_value=True)
130+
@patch("pr_comment._find_existing_comments", return_value=[])
131+
def test_post_pr_comments_new_conflict_keys_reverse_pair(
132+
self, _mock_find, mock_post
133+
):
134+
"""The reversed (other, pr_number) tuple should also be detected as new."""
135+
conflict = _make_comment_conflict()
136+
conflicts = {"org/repo": [conflict]}
137+
138+
gh = MagicMock()
139+
gh.repository.return_value = MagicMock()
140+
141+
# Only the reversed tuple is present — the disjunction's second half must fire.
142+
new_keys = {(2, 1)}
143+
result = pr_comment.post_pr_comments(conflicts, gh, new_conflict_keys=new_keys)
144+
145+
self.assertTrue(result)
146+
self.assertEqual(mock_post.call_count, 2)
147+
148+
bodies_by_pr = {call.args[1]: call.args[2] for call in mock_post.call_args_list}
149+
# Without the (other, pr_number) branch, neither body would contain 🆕.
150+
self.assertIn("🆕", bodies_by_pr[1])
151+
self.assertIn("🆕", bodies_by_pr[2])
152+
153+
@patch("pr_comment._post_comment", return_value=False)
154+
@patch("pr_comment._find_existing_comments", return_value=[])
155+
def test_post_pr_comments_post_failure_returns_false(self, _mock_find, _mock_post):
156+
"""If posting a new comment fails, the overall return should be False."""
157+
conflict = _make_comment_conflict()
158+
conflicts = {"org/repo": [conflict]}
159+
160+
gh = MagicMock()
161+
gh.repository.return_value = MagicMock()
162+
163+
result = pr_comment.post_pr_comments(conflicts, gh)
164+
self.assertFalse(result)
165+
166+
@patch("pr_comment._update_comment", return_value=False)
167+
@patch("pr_comment._find_existing_comments")
168+
def test_post_pr_comments_update_failure_returns_false(
169+
self, mock_find, _mock_update
170+
):
171+
"""If updating an existing comment fails, the overall return should be False."""
172+
conflict = _make_comment_conflict()
173+
conflicts = {"org/repo": [conflict]}
174+
175+
mock_find.return_value = [MagicMock()]
176+
177+
gh = MagicMock()
178+
gh.repository.return_value = MagicMock()
179+
180+
result = pr_comment.post_pr_comments(conflicts, gh)
181+
self.assertFalse(result)
182+
107183

108184
class TestFindExistingComments(unittest.TestCase):
109185
"""Tests for the _find_existing_comments function."""

‎test_slack_notify.py‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,30 @@ def test_send_slack_notification_empty_conflict_lists(self):
9090
)
9191
self.assertTrue(result)
9292

93+
@patch("slack_notify.cluster_conflicts", wraps=slack_notify.cluster_conflicts)
94+
@patch("slack_notify.post_to_slack", return_value=True)
95+
def test_send_slack_notification_skips_repo_with_empty_list(
96+
self, mock_post, mock_cluster
97+
):
98+
"""A repo with an empty conflict list mixed with non-empty repos is skipped.
99+
100+
The `continue` at slack_notify.py:42-43 short-circuits before cluster_conflicts
101+
is called, so a real skip is observable as cluster_conflicts being called once
102+
(for the non-empty repo) rather than twice.
103+
"""
104+
conflicts = {
105+
"org/empty-repo": [],
106+
"org/repo": [_make_conflict()],
107+
}
108+
result = slack_notify.send_slack_notification(
109+
"https://hooks.slack.com/test", conflicts
110+
)
111+
self.assertTrue(result)
112+
self.assertEqual(mock_post.call_count, 1)
113+
# cluster_conflicts must only run for the non-empty repo; deleting the
114+
# `continue` would cause it to run twice (once with `[]`).
115+
self.assertEqual(mock_cluster.call_count, 1)
116+
93117
@patch("slack_notify.post_to_slack")
94118
def test_send_slack_notification_dry_run(self, mock_post):
95119
"""Dry-run mode should return True but never call post_to_slack."""

0 commit comments

Comments
 (0)