[IOSP-300] Increase status checks timeout and report it properly (#38)

* IOSP-300: Increase status checks timeout to 1h30

+ make it configurable via Env variables
+ Report timeout error instead of blocked when status timed out

* Ensure timeout is properly reported in comment

* swift test --generate-linuxmain

Sources/App/Extensions/EnvironmentProperties.swift

@@ -26,6 +26,11 @@ extension Environment {
         return ["yes", "1", "true"].contains(stringValue.lowercased())
     }
 
+    static func statusChecksTimeout() throws -> TimeInterval? {
+        let value: String = try Environment.get("STATUS_CHECKS_TIMEOUT")
+        return TimeInterval(value)
+    }
+
     static func mergeLabel() throws -> PullRequest.Label {
         return PullRequest.Label(name: try Environment.get("MERGE_LABEL"))
     }

Sources/App/configure.swift

@@ -42,7 +42,8 @@ private func makeMergeService(with logger: LoggerProtocol, _ gitHubEventsService
     return MergeService(
         integrationLabel: try Environment.mergeLabel(),
         topPriorityLabels: try Environment.topPriorityLabels(),
-        requiresAllStatusChecks: try Environment.requiresAllGitHubStatusChecks(), 
+        requiresAllStatusChecks: try Environment.requiresAllGitHubStatusChecks(),
+        statusChecksTimeout: try Environment.statusChecksTimeout() ?? 90.minutes,
         logger: logger,
         gitHubAPI: gitHubAPI,
         gitHubEvents: gitHubEventsService

Sources/Bot/Services/MergeService.swift

@@ -22,7 +22,7 @@ public final class MergeService {
         integrationLabel: PullRequest.Label,
         topPriorityLabels: [PullRequest.Label],
         requiresAllStatusChecks: Bool,
-        statusChecksTimeout: TimeInterval = 60.minutes,
+        statusChecksTimeout: TimeInterval,
         logger: LoggerProtocol,
         gitHubAPI: GitHubAPIProtocol,
         gitHubEvents: GitHubEventsServiceProtocol,
@@ -316,7 +316,7 @@ extension MergeService {
                     logger.log("📣 Status check `\(change.context)` finished with result: `\(change.state)` (SHA: `\(change.sha)`)")
                 }
                 // Checks can complete and lead to new checks which can be included posteriorly leading to a small time
-                // window where all checks have passed but just until the next check is added and stars running. This
+                // window where all checks have passed but just until the next check is added and starts running. This
                 // hopefully prevents those false positives by making sure we wait some time before checking if all
                 // checks have passed
                 .debounce(60.0, on: scheduler)
@@ -344,7 +344,7 @@ extension MergeService {
                 .timeout(after: context.statusChecksTimeout, raising: TimeoutError.timedOut, on: scheduler)
                 .flatMapError { error in
                     switch error {
-                    case .timedOut: return .value(.statusChecksDidComplete(.failed(context.pullRequestMetadata)))
+                    case .timedOut: return .value(.statusChecksDidComplete(.timedOut(context.pullRequestMetadata)))
                     }
                 }
         }
@@ -489,6 +489,7 @@ extension MergeService {
         case synchronizationFailed
         case checkingCommitChecksFailed
         case checksFailing
+        case timedOut
         case blocked
         case unknown
     }
@@ -620,6 +621,7 @@ extension MergeService {
         enum StatusChecksResult {
             case failed(PullRequestMetadata)
             case passed(PullRequestMetadata)
+            case timedOut(PullRequestMetadata)
         }
 
         enum IntegrationStatus {
@@ -690,6 +692,8 @@ extension MergeService.State {
             return self.with(status: .integrating(pullRequest))
         case let .statusChecksDidComplete(.failed(pullRequest)):
             return self.with(status: .integrationFailed(pullRequest, .checksFailing))
+        case let .statusChecksDidComplete(.timedOut(pullRequest)):
+            return self.with(status: .integrationFailed(pullRequest, .timedOut))
         case let .pullRequestDidChange(.exclude(pullRequestExcluded)) where metadata.reference.number == pullRequestExcluded.number:
             return self.with(status: .ready)
         default:

Tests/BotTests/MergeService/MergeServiceTests.swift

@@ -589,7 +589,9 @@ class MergeServiceTests: XCTestCase {
                 .getPullRequest { _ in MergeServiceFixture.defaultTarget },
                 .postComment { _, _ in },
                 .mergeIntoBranch { _, _ in .success },
-                .postComment { _, _ in },
+                .postComment { comment, _ in
+                    expect(comment) == "@John Doe unfortunately the integration failed with code: `timedOut`."
+                },
                 .removeLabel { _, _ in }
                 ],
             when: { service, scheduler in
@@ -608,7 +610,7 @@ class MergeServiceTests: XCTestCase {
                     MergeService.State.stub(status: .ready, pullRequests: [MergeServiceFixture.defaultTarget.reference]),
                     MergeService.State.stub(status: .integrating(MergeServiceFixture.defaultTarget)),
                     MergeService.State.stub(status: .runningStatusChecks(MergeServiceFixture.defaultTarget.with(mergeState: .blocked))),
-                    MergeService.State.stub(status: .integrationFailed(MergeServiceFixture.defaultTarget.with(mergeState: .blocked), .checksFailing)),
+                    MergeService.State.stub(status: .integrationFailed(MergeServiceFixture.defaultTarget.with(mergeState: .blocked), .timedOut)),
                     MergeService.State.stub(status: .ready),
                     MergeService.State.stub(status: .idle)
                 ]

Tests/BotTests/XCTestManifests.swift

@@ -10,6 +10,7 @@ extension GitHubAPITests {
         ("test_fetch_commit_status", test_fetch_commit_status),
         ("test_fetch_pull_request_number", test_fetch_pull_request_number),
         ("test_fetch_pull_requests", test_fetch_pull_requests),
+        ("test_fetch_required_status_checks", test_fetch_required_status_checks),
         ("test_merge_branch_up_to_date", test_merge_branch_up_to_date),
         ("test_merge_branch_with_success", test_merge_branch_with_success),
         ("test_merge_pull_request", test_merge_pull_request),
@@ -73,6 +74,8 @@ extension MergeServiceTests {
         ("test_pull_request_with_integration_label_and_conflicts", test_pull_request_with_integration_label_and_conflicts),
         ("test_pull_request_with_integration_label_and_ready_to_merge", test_pull_request_with_integration_label_and_ready_to_merge),
         ("test_pull_request_with_multiple_status_checks", test_pull_request_with_multiple_status_checks),
+        ("test_pull_request_with_non_required_failed_status_checks_requiresAllStatusChecks_off", test_pull_request_with_non_required_failed_status_checks_requiresAllStatusChecks_off),
+        ("test_pull_request_with_non_required_failed_status_checks_requiresAllStatusChecks_on", test_pull_request_with_non_required_failed_status_checks_requiresAllStatusChecks_on),
         ("test_pull_request_with_status_checks_failing", test_pull_request_with_status_checks_failing),
         ("test_pull_requests_receive_feedback_when_accepted", test_pull_requests_receive_feedback_when_accepted),
         ("test_removing_the_integration_label_during_integration", test_removing_the_integration_label_during_integration),