From a2e652d8887dbfdd319db0d063c39454df43c934 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Sat, 10 Jul 2021 19:01:45 -0400 Subject: [PATCH 1/4] fix(bot): handling of duplicate check suites Previously if a user created duplicate check suites with the same check run name, Kodiak would consider all of the check suites, not just the most recent one. So if the check suite had failed previously, but the latest check suite passed, Kodiak would incorrectly mark the PR as blocked for merging. Now Kodiak only considers the most recent check run name. --- bot/kodiak/evaluation.py | 13 ++++++- bot/kodiak/test_evaluation.py | 8 +++-- .../tests/evaluation/test_check_runs.py | 35 ++++++++++++++++++- 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/bot/kodiak/evaluation.py b/bot/kodiak/evaluation.py index 8d2d67f4a..402dc0e5b 100644 --- a/bot/kodiak/evaluation.py +++ b/bot/kodiak/evaluation.py @@ -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()) + + 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) diff --git a/bot/kodiak/test_evaluation.py b/bot/kodiak/test_evaluation.py index 7badec07e..e1be2574a 100644 --- a/bot/kodiak/test_evaluation.py +++ b/bot/kodiak/test_evaluation.py @@ -282,8 +282,12 @@ def create_context() -> StatusContext: return StatusContext(context="ci/api", state=StatusState.SUCCESS) -def create_check_run() -> CheckRun: - return CheckRun(name="WIP (beta)", conclusion=CheckConclusionState.SUCCESS) +def create_check_run( + *, + name: str = "WIP (beta)", + conclusion: CheckConclusionState = CheckConclusionState.SUCCESS, +) -> CheckRun: + return CheckRun(name=name, conclusion=conclusion) def create_review_request() -> PRReviewRequest: diff --git a/bot/kodiak/tests/evaluation/test_check_runs.py b/bot/kodiak/tests/evaluation/test_check_runs.py index 78bfb97a1..4bc567ffa 100644 --- a/bot/kodiak/tests/evaluation/test_check_runs.py +++ b/bot/kodiak/tests/evaluation/test_check_runs.py @@ -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,31 @@ 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 + + +async def test_duplicate_check_suites() -> None: + 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 + ), + ], + ) + assert "enqueued for merge" in api.set_status.calls[0]["msg"] + assert api.queue_for_merge.call_count == 1 From cf011c8a68743db8811603c4022ff4aea77f8353 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Sat, 10 Jul 2021 19:03:03 -0400 Subject: [PATCH 2/4] doc --- bot/kodiak/tests/evaluation/test_check_runs.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bot/kodiak/tests/evaluation/test_check_runs.py b/bot/kodiak/tests/evaluation/test_check_runs.py index 4bc567ffa..6e4b1e0b9 100644 --- a/bot/kodiak/tests/evaluation/test_check_runs.py +++ b/bot/kodiak/tests/evaluation/test_check_runs.py @@ -570,6 +570,12 @@ async def test_mergeable_update_always_no_require_automerge_label_missing_label( 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 From 7c2a91a19fe08aac9ea15de4c46717fe2be2ca1d Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Sat, 10 Jul 2021 19:03:45 -0400 Subject: [PATCH 3/4] fix --- bot/kodiak/tests/evaluation/test_check_runs.py | 1 + 1 file changed, 1 insertion(+) diff --git a/bot/kodiak/tests/evaluation/test_check_runs.py b/bot/kodiak/tests/evaluation/test_check_runs.py index 6e4b1e0b9..36eb6c4d8 100644 --- a/bot/kodiak/tests/evaluation/test_check_runs.py +++ b/bot/kodiak/tests/evaluation/test_check_runs.py @@ -569,6 +569,7 @@ async def test_mergeable_update_always_no_require_automerge_label_missing_label( 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 From f6b2966eeb0b8ee21dc71e72e2134cb14f7034d8 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Sat, 10 Jul 2021 21:32:06 -0400 Subject: [PATCH 4/4] CR --- bot/kodiak/evaluation.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/bot/kodiak/evaluation.py b/bot/kodiak/evaluation.py index 402dc0e5b..86ef1b77f 100644 --- a/bot/kodiak/evaluation.py +++ b/bot/kodiak/evaluation.py @@ -263,11 +263,9 @@ 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()) +def deduplicate_check_runs(check_runs: Iterable[CheckRun]) -> Iterable[CheckRun]: + check_run_map = {check_run.name: check_run for check_run in check_runs} + return check_run_map.values() class PRAPI(Protocol):