[IOSP-436] Check for any pending checks before integrating clean PR (#43

) * use manually comuted states * check if any status check pending and then wait for it, otherwise check PR status as we did before * added tests * Fix test flakiness on JSON comparison * Ensure test flakiness is not due to not using TestScheduler * revert changes to tests manifest as some tests are failing on CI * Revert "Fix test flakiness on JSON comparison" This reverts commit 68c7b3c. * Apply suggestions from code review Co-Authored-By: Daniel Spindelbauer <[email protected]> Co-Authored-By: Olivier Halligon <[email protected]>
babylonhealth · Dec 9, 2019 · 85ef942 · 85ef942
1 parent 41d6742
commit 85ef942
Show file tree

Hide file tree

Showing 8 changed files with 106 additions and 21 deletions.
diff --git a/Sources/Bot/API/GitHubAPIProtocol.swift b/Sources/Bot/API/GitHubAPIProtocol.swift
@@ -12,6 +12,8 @@ public protocol GitHubAPIProtocol {
 
     func fetchRequiredStatusChecks(for branch: PullRequest.Branch) -> SignalProducer<RequiredStatusChecks, AnyError>
 
+    func fetchAllStatusChecks(for pullRequest: PullRequest) -> SignalProducer<[PullRequest.StatusCheck], AnyError>
+
     /// Merges one branch into another.
     ///
     /// - SeeAlso: https://developer.github.com/v3/repos/merging/

diff --git a/Sources/Bot/API/Repository.swift b/Sources/Bot/API/Repository.swift
@@ -59,6 +59,14 @@ extension Repository {
         )
     }
 
+    func allStatusChecks(for pullRequest: PullRequest) -> Resource<[PullRequest.StatusCheck]> {
+        return Resource(
+            method: .GET,
+            path: path(for: "commits/\(pullRequest.source.sha)/statuses"),
+            decoder: decode
+        )
+    }
+
     func deleteBranch(branch: PullRequest.Branch) -> Resource<Void> {
         return Resource(
             method: .DELETE,

diff --git a/Sources/Bot/API/RepositoryAPI.swift b/Sources/Bot/API/RepositoryAPI.swift
@@ -34,6 +34,11 @@ public struct RepositoryAPI: GitHubAPIProtocol {
             .mapError(AnyError.init)
     }
 
+    public func fetchAllStatusChecks(for pullRequest: PullRequest) -> SignalProducer<[PullRequest.StatusCheck], AnyError> {
+        return client.request(repository.allStatusChecks(for: pullRequest))
+            .mapError(AnyError.init)
+    }
+
     public func merge(head: PullRequest.Branch, into base: PullRequest.Branch) -> SignalProducer<MergeResult, AnyError> {
         return client.request(repository.merge(head: head, into: base))
             .mapError(AnyError.init)

diff --git a/Sources/Bot/Models/PullRequest.swift b/Sources/Bot/Models/PullRequest.swift
@@ -32,6 +32,14 @@ extension PullRequest {
     }
 }
 
+extension PullRequest {
+    public struct StatusCheck: Equatable, Decodable {
+        public let state: CommitState.State
+        public let context: String
+        public let description: String
+    }
+}
+
 extension PullRequest {
     public enum Action: String, Decodable {
         case assigned

diff --git a/Sources/Bot/Services/MergeService.swift b/Sources/Bot/Services/MergeService.swift
@@ -404,27 +404,37 @@ extension MergeService {
                     .observe(on: scheduler)
             case .blocked,
                  .unstable:
-                return github.fetchCommitStatus(for: metadata.reference)
-                    .flatMap(.latest) { commitStatus -> SignalProducer<Event, AnyError> in
-                        switch commitStatus.state {
-                        case .pending:
-                            return .value(.integrationDidChangeStatus(.updating, metadata))
-                        case .failure:
-                            return .value(.integrationDidChangeStatus(.failed(.checksFailing), metadata))
-                        case  .success:
-                            return github.fetchPullRequest(number: metadata.reference.number)
-                                .map { metadata in
-                                    switch metadata.mergeState {
-                                    case .clean:
-                                        return .retryIntegration(metadata)
-                                    default:
-                                        return .integrationDidChangeStatus(.failed(.blocked), metadata)
+                let pullRequest = metadata.reference
+                return github.fetchAllStatusChecks(for: pullRequest).map { statusChecks -> Bool in
+                    return statusChecks.map({ $0.state }).contains(.pending)
+                }.flatMap(.latest) { pendingStatusChecks -> SignalProducer<Event, AnyError> in
+                    if pendingStatusChecks {
+                        return .value(.integrationDidChangeStatus(.updating, metadata))
+                    } else {
+                        return github.fetchCommitStatus(for: metadata.reference)
+                            .flatMap(.latest) { commitStatus -> SignalProducer<Event, AnyError> in
+                                switch commitStatus.state {
+                                case .pending:
+                                    return .value(.integrationDidChangeStatus(.updating, metadata))
+                                case .failure:
+                                    return .value(.integrationDidChangeStatus(.failed(.checksFailing), metadata))
+                                case  .success:
+                                    return github.fetchPullRequest(number: metadata.reference.number)
+                                        .map { metadata in
+                                            switch metadata.mergeState {
+                                            case .clean:
+                                                return .retryIntegration(metadata)
+                                            default:
+                                                return .integrationDidChangeStatus(.failed(.blocked), metadata)
+                                            }
                                     }
                             }
                         }
+                        .observe(on: scheduler)
                     }
-                    .flatMapError { _ in .value(Event.integrationDidChangeStatus(.failed(.checkingCommitChecksFailed), metadata)) }
-                    .observe(on: scheduler)
+                }
+                .flatMapError { _ in .value(Event.integrationDidChangeStatus(.failed(.checkingCommitChecksFailed), metadata)) }
+                .observe(on: scheduler)
             case .dirty:
                 return SignalProducer(value: Event.integrationDidChangeStatus(.failed(.conflicts), metadata))
                     .observe(on: scheduler)
@@ -448,7 +458,7 @@ extension MergeService {
                     }
                     .retry(upTo: 4, interval: 30.0, on: scheduler)
                     .flatMapError { _ in .value(Event.integrationDidChangeStatus(.failed(.unknown), metadata)) }
-
+                    .observe(on: scheduler)
             }
         }
     }

diff --git a/Tests/BotTests/MergeService/MergeServiceTests.swift b/Tests/BotTests/MergeService/MergeServiceTests.swift
@@ -191,12 +191,13 @@ class MergeServiceTests: XCTestCase {
         )
     }
 
-    func test_pull_request_blocked_with_successful_status() {
+    func test_pull_request_blocked_with_successful_status_no_pending_checks() {
         perform(
             stubs: [
                 .getPullRequests { [MergeServiceFixture.defaultTarget.reference] },
                 .getPullRequest { _ in MergeServiceFixture.defaultTarget.with(mergeState: .blocked) },
                 .postComment { _, _ in },
+                .getAllStatusChecks { _ in [.init(state: .success, context: "", description: "")]},
                 .getCommitStatus { _ in CommitState.stub(states: [.success]) },
                 .getPullRequest { _ in MergeServiceFixture.defaultTarget.with(mergeState: .clean) },
                 .mergePullRequest { _ in },
@@ -224,6 +225,47 @@ class MergeServiceTests: XCTestCase {
         )
     }
 
+    func test_pull_request_blocked_with_successful_status_pending_checks() {
+        perform(
+            stubs: [
+                .getPullRequests { [MergeServiceFixture.defaultTarget.reference] },
+                .getPullRequest { _ in MergeServiceFixture.defaultTarget.with(mergeState: .blocked) },
+                .postComment { _, _ in },
+                .getAllStatusChecks { _ in
+                    [
+                        .init(state: .pending, context: "test", description: ""),
+                        .init(state: .success, context: "build", description: "")
+                    ]
+                },
+                .getPullRequest { _ in MergeServiceFixture.defaultTarget.with(mergeState: .clean) },
+                .getCommitStatus { _ in CommitState.stub(states: [.success]) },
+                .mergePullRequest { _ in },
+                .deleteBranch { _ in }
+            ],
+            when: { service, scheduler in
+                scheduler.advance()
+
+                service.sendStatusEvent(state: .success)
+
+                scheduler.advance(by: .seconds(90))
+
+            },
+            assert: {
+                expect($0) == [
+                    .created(branch: MergeServiceFixture.defaultTargetBranch),
+                    .state(.stub(status: .starting)),
+                    .state(.stub(status: .ready, pullRequests: [MergeServiceFixture.defaultTarget.reference])),
+                    .state(.stub(status: .integrating(MergeServiceFixture.defaultTarget.with(mergeState: .blocked)))),
+                    .state(.stub(status: .runningStatusChecks(MergeServiceFixture.defaultTarget.with(mergeState: .blocked)))),
+                    .state(.stub(status: .integrating(MergeServiceFixture.defaultTarget.with(mergeState: .clean)))),
+                    .state(.stub(status: .ready)),
+                    .state(.stub(status: .idle)),
+                    .destroyed(branch: MergeServiceFixture.defaultTargetBranch)
+                ]
+            }
+        )
+    }
+
     func test_resuming_after_labelling_a_pull_request() {
 
         let target = PullRequestMetadata.stub(number: 1, headRef: MergeServiceFixture.defaultBranch, labels: [], mergeState: .clean)

diff --git a/Tests/BotTests/Mocks/MockGitHubAPI.swift b/Tests/BotTests/Mocks/MockGitHubAPI.swift
@@ -5,12 +5,12 @@ import Nimble
 @testable import Bot
 
 struct MockGitHubAPI: GitHubAPIProtocol {
-
     enum Stubs {
         case getPullRequests(() -> [PullRequest])
         case getPullRequest((UInt) -> PullRequestMetadata)
         case getCommitStatus((PullRequest) -> CommitState)
         case getRequiredStatusChecks((PullRequest.Branch) -> RequiredStatusChecks)
+        case getAllStatusChecks((PullRequest) -> [PullRequest.StatusCheck])
         case mergePullRequest((PullRequest) -> Void)
         case mergeIntoBranch((PullRequest.Branch, PullRequest.Branch) -> MergeResult)
         case deleteBranch((PullRequest.Branch) -> Void)
@@ -62,6 +62,15 @@ struct MockGitHubAPI: GitHubAPIProtocol {
         }
     }
 
+    func fetchAllStatusChecks(for pullRequest: PullRequest) -> SignalProducer<[PullRequest.StatusCheck], AnyError> {
+        switch nextStub() {
+        case let .getAllStatusChecks(handler):
+            return SignalProducer(value: handler(pullRequest))
+        default:
+            fatalError("Stub not found")
+        }
+    }
+
     func merge(head: PullRequest.Branch, into base: PullRequest.Branch) -> SignalProducer<MergeResult, AnyError> {
         switch nextStub() {
         case let .mergeIntoBranch(handler):

diff --git a/Tests/BotTests/XCTestManifests.swift b/Tests/BotTests/XCTestManifests.swift
@@ -65,7 +65,8 @@ extension MergeServiceTests {
         ("test_excluding_pull_request_in_the_queue", test_excluding_pull_request_in_the_queue),
         ("test_multiple_pull_requests_with_integration_label_and_ready_to_merge", test_multiple_pull_requests_with_integration_label_and_ready_to_merge),
         ("test_no_pull_requests_with_integration_label", test_no_pull_requests_with_integration_label),
-        ("test_pull_request_blocked_with_successful_status", test_pull_request_blocked_with_successful_status),
+        ("test_pull_request_blocked_with_successful_status_no_pending_checks", test_pull_request_blocked_with_successful_status_no_pending_checks),
+        ("test_pull_request_blocked_with_successful_status_pending_checks", test_pull_request_blocked_with_successful_status_pending_checks),
         ("test_pull_request_does_not_fail_prematurely_if_checks_complete_before_adding_the_following_checks", test_pull_request_does_not_fail_prematurely_if_checks_complete_before_adding_the_following_checks),
         ("test_pull_request_fails_integration_after_timeout", test_pull_request_fails_integration_after_timeout),
         ("test_pull_request_with_an_initial_unknown_state_with_recover", test_pull_request_with_an_initial_unknown_state_with_recover),