-
-
Notifications
You must be signed in to change notification settings - Fork 63
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(bot): handling of duplicate check suites #688
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,9 @@ | ||||||||||||||||||||
from __future__ import annotations | ||||||||||||||||||||
|
||||||||||||||||||||
import asyncio | ||||||||||||||||||||
import textwrap | ||||||||||||||||||||
from collections import defaultdict | ||||||||||||||||||||
from collections.abc import Iterable | ||||||||||||||||||||
from dataclasses import dataclass | ||||||||||||||||||||
from typing import Dict, List, MutableMapping, Optional, Sequence, Set, Union | ||||||||||||||||||||
|
||||||||||||||||||||
|
@@ -260,6 +263,13 @@ def review_status(reviews: List[PRReview]) -> PRReviewState: | |||||||||||||||||||
return status | ||||||||||||||||||||
|
||||||||||||||||||||
|
||||||||||||||||||||
def deduplicate_check_runs(check_runs: Iterable[CheckRun]) -> List[CheckRun]: | ||||||||||||||||||||
check_run_map = dict() | ||||||||||||||||||||
for check_run in check_runs: | ||||||||||||||||||||
check_run_map[check_run.name] = check_run | ||||||||||||||||||||
return list(check_run_map.values()) | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||
|
||||||||||||||||||||
|
||||||||||||||||||||
class PRAPI(Protocol): | ||||||||||||||||||||
async def dequeue(self) -> None: | ||||||||||||||||||||
... | ||||||||||||||||||||
|
@@ -882,7 +892,8 @@ async def set_status(msg: str, markdown_content: Optional[str] = None) -> None: | |||||||||||||||||||
else: | ||||||||||||||||||||
assert status_context.state == StatusState.SUCCESS | ||||||||||||||||||||
passing_contexts.append(status_context.context) | ||||||||||||||||||||
for check_run in check_runs: | ||||||||||||||||||||
|
||||||||||||||||||||
for check_run in deduplicate_check_runs(check_runs): | ||||||||||||||||||||
if ( | ||||||||||||||||||||
check_run.name in config.merge.dont_wait_on_status_checks | ||||||||||||||||||||
and check_run.conclusion in (None, CheckConclusionState.NEUTRAL) | ||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,13 @@ | ||
""" | ||
Tests for flows that depend on status checks or check_runs. | ||
""" | ||
import pytest | ||
|
||
from kodiak.errors import PollForever, RetryForSkippableChecks | ||
from kodiak.queries import CheckConclusionState, MergeStateStatus, StatusState | ||
from kodiak.queries import StatusState | ||
from kodiak.test_evaluation import ( | ||
CheckConclusionState, | ||
MergeStateStatus, | ||
create_api, | ||
create_branch_protection, | ||
create_check_run, | ||
|
@@ -562,3 +567,38 @@ async def test_mergeable_update_always_no_require_automerge_label_missing_label( | |
assert api.queue_for_merge.call_count == 0 | ||
assert api.merge.call_count == 0 | ||
assert api.dequeue.call_count == 0 | ||
|
||
|
||
@pytest.mark.asyncio | ||
async def test_duplicate_check_suites() -> None: | ||
""" | ||
Kodiak should only consider the most recent check run when evaluating a PR | ||
for merging. | ||
|
||
Regression test. | ||
""" | ||
api = create_api() | ||
pull_request = create_pull_request() | ||
pull_request.mergeStateStatus = MergeStateStatus.BEHIND | ||
branch_protection = create_branch_protection() | ||
branch_protection.requiresStatusChecks = True | ||
branch_protection.requiredStatusCheckContexts = ["Pre-merge checks"] | ||
mergeable = create_mergeable() | ||
await mergeable( | ||
api=api, | ||
pull_request=pull_request, | ||
branch_protection=branch_protection, | ||
check_runs=[ | ||
create_check_run( | ||
name="Pre-merge checks", conclusion=CheckConclusionState.NEUTRAL | ||
), | ||
create_check_run( | ||
name="Pre-merge checks", conclusion=CheckConclusionState.FAILURE | ||
), | ||
create_check_run( | ||
name="Pre-merge checks", conclusion=CheckConclusionState.SUCCESS | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does the GitHub API order the results with the most recent ones last? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I opened a support ticket about that but my assumption here is yes |
||
), | ||
], | ||
) | ||
assert "enqueued for merge" in api.set_status.calls[0]["msg"] | ||
assert api.queue_for_merge.call_count == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.