From fcf891480689e46e4eb8827ae445d95dbd172c53 Mon Sep 17 00:00:00 2001 From: Luke LaBonte Date: Fri, 8 May 2026 18:25:06 -0500 Subject: [PATCH 1/4] Respect required CI checks Show approval-gated non-blocking checks separately while keeping merge status based on required GitHub checks. --- MonitorLizard/Models/PullRequest.swift | 136 ++++++++- MonitorLizard/Models/StatusCheck.swift | 21 +- MonitorLizard/Services/GitHubService.swift | 169 ++++++++--- MonitorLizard/Views/PRRowView.swift | 31 +- MonitorLizardTests/GitHubServiceTests.swift | 312 ++++++++++++++++++++ 5 files changed, 629 insertions(+), 40 deletions(-) diff --git a/MonitorLizard/Models/PullRequest.swift b/MonitorLizard/Models/PullRequest.swift index c23b9ee..f48148f 100644 --- a/MonitorLizard/Models/PullRequest.swift +++ b/MonitorLizard/Models/PullRequest.swift @@ -91,6 +91,94 @@ struct PullRequest: Identifiable, Hashable { } } +enum NonBlockingCheckState: Hashable { + case failed + case waitingForApproval + case running + case queued + case pending + case passed +} + +struct NonBlockingCheckSummary: Hashable { + struct Segment: Identifiable, Hashable { + let state: NonBlockingCheckState + let count: Int + + var id: NonBlockingCheckState { state } + + var text: String { + switch state { + case .failed: + return count == 1 ? "1 failed" : "\(count) failed" + case .waitingForApproval: + return count == 1 ? "1 waiting for approval" : "\(count) waiting for approval" + case .running: + return count == 1 ? "1 running" : "\(count) running" + case .queued: + return count == 1 ? "1 queued" : "\(count) queued" + case .pending: + return count == 1 ? "1 pending" : "\(count) pending" + case .passed: + return count == 1 ? "1 passed" : "\(count) passed" + } + } + } + + let segments: [Segment] +} + +extension PullRequest { + var nonBlockingCheckSummary: NonBlockingCheckSummary? { + let nonBlockingChecks = statusChecks.filter(\.isNonBlocking) + guard !nonBlockingChecks.isEmpty else { return nil } + + let counts = Dictionary(grouping: nonBlockingChecks, by: nonBlockingCheckState(for:)) + .mapValues(\.count) + let needsAttention = [ + NonBlockingCheckState.failed, + .waitingForApproval, + .running, + .queued, + .pending, + ].contains { (counts[$0] ?? 0) > 0 } + guard needsAttention else { return nil } + + let segments = [ + NonBlockingCheckState.failed, + .waitingForApproval, + .running, + .queued, + .pending, + .passed, + ].compactMap { state -> NonBlockingCheckSummary.Segment? in + guard let count = counts[state], count > 0 else { return nil } + return NonBlockingCheckSummary.Segment(state: state, count: count) + } + + return NonBlockingCheckSummary(segments: segments) + } + + private func nonBlockingCheckState(for check: StatusCheck) -> NonBlockingCheckState { + switch check.status { + case .failure, .error: + return .failed + case .waiting: + return .waitingForApproval + case .running: + return .running + case .queued: + return .queued + case .pending: + return .pending + case .success: + return .passed + case .skipped: + return .passed + } + } +} + /// Identifies a specific PR for use in batch status requests. struct PRStatusRequest: Hashable { let owner: String @@ -165,6 +253,7 @@ struct BatchPRStatusResponse: Codable { let reviewDecision: String? let latestReviews: ReviewConnection? let reviewRequests: ReviewRequestConnection? + let baseRef: BaseRef? /// Wraps the raw GraphQL `statusCheckRollup { contexts { nodes [...] } }` shape. struct StatusCheckRollupWrapper: Codable { @@ -191,6 +280,26 @@ struct BatchPRStatusResponse: Codable { } } + struct BaseRef: Codable { + let branchProtectionRule: BranchProtectionRule? + } + + struct BranchProtectionRule: Codable { + let requiredStatusCheckContexts: [String]? + let requiredStatusChecks: [RequiredStatusCheck]? + + struct RequiredStatusCheck: Codable { + let context: String + } + } + + var requiredStatusCheckContexts: [String]? { + guard let rule = baseRef?.branchProtectionRule else { return nil } + let contexts = rule.requiredStatusCheckContexts ?? [] + let checkContexts = rule.requiredStatusChecks?.map(\.context) ?? [] + return Array(Set(contexts + checkContexts)) + } + /// Converts to GHPRDetailResponse so existing status-parsing logic can be reused. func toDetailResponse() -> GHPRDetailResponse { let flatRequests = reviewRequests?.nodes?.map { @@ -203,7 +312,8 @@ struct BatchPRStatusResponse: Codable { mergeStateStatus: mergeStateStatus, reviewDecision: reviewDecision, latestReviews: latestReviews?.nodes, - reviewRequests: flatRequests + reviewRequests: flatRequests, + requiredStatusCheckContexts: requiredStatusCheckContexts ) } } @@ -226,6 +336,27 @@ struct GHPRDetailResponse: Codable { let reviewDecision: String? let latestReviews: [Review]? let reviewRequests: [ReviewRequest]? + let requiredStatusCheckContexts: [String]? + + init( + headRefName: String, + statusCheckRollup: [StatusCheck]?, + mergeable: String?, + mergeStateStatus: String?, + reviewDecision: String?, + latestReviews: [Review]?, + reviewRequests: [ReviewRequest]?, + requiredStatusCheckContexts: [String]? = nil + ) { + self.headRefName = headRefName + self.statusCheckRollup = statusCheckRollup + self.mergeable = mergeable + self.mergeStateStatus = mergeStateStatus + self.reviewDecision = reviewDecision + self.latestReviews = latestReviews + self.reviewRequests = reviewRequests + self.requiredStatusCheckContexts = requiredStatusCheckContexts + } struct Review: Codable { let author: ReviewAuthor? @@ -249,9 +380,10 @@ struct GHPRDetailResponse: Codable { let __typename: String let detailsUrl: String? let targetUrl: String? + let isRequired: Bool? private enum CodingKeys: String, CodingKey { - case name, context, status, state, conclusion, __typename, detailsUrl, targetUrl + case name, context, status, state, conclusion, __typename, detailsUrl, targetUrl, isRequired } } } diff --git a/MonitorLizard/Models/StatusCheck.swift b/MonitorLizard/Models/StatusCheck.swift index 6ac35fa..be38c37 100644 --- a/MonitorLizard/Models/StatusCheck.swift +++ b/MonitorLizard/Models/StatusCheck.swift @@ -9,6 +9,9 @@ import SwiftUI enum CheckStatus: String, Codable { case pending + case queued + case running + case waiting case success case failure case error @@ -18,6 +21,12 @@ enum CheckStatus: String, Codable { switch self { case .pending: return "⏳" + case .queued: + return "⏳" + case .running: + return "🔄" + case .waiting: + return "⏸" case .success: return "✅" case .failure: @@ -33,6 +42,12 @@ enum CheckStatus: String, Codable { switch self { case .pending: return .orange + case .queued: + return .blue + case .running: + return .blue + case .waiting: + return .orange case .success: return .green case .failure: @@ -50,11 +65,15 @@ struct StatusCheck: Identifiable, Codable, Hashable { let name: String let status: CheckStatus let detailsUrl: String? + let isRequired: Bool? + let isNonBlocking: Bool - init(id: String, name: String, status: CheckStatus, detailsUrl: String?) { + init(id: String, name: String, status: CheckStatus, detailsUrl: String?, isRequired: Bool? = nil, isNonBlocking: Bool = false) { self.id = id self.name = name self.status = status self.detailsUrl = detailsUrl + self.isRequired = isRequired + self.isNonBlocking = isNonBlocking } } diff --git a/MonitorLizard/Services/GitHubService.swift b/MonitorLizard/Services/GitHubService.swift index 156772f..0fee04b 100644 --- a/MonitorLizard/Services/GitHubService.swift +++ b/MonitorLizard/Services/GitHubService.swift @@ -102,6 +102,14 @@ class GitHubService: ObservableObject { pr\(index): repository(owner: "\(req.owner)", name: "\(req.repo)") { pullRequest(number: \(req.number)) { headRefName + baseRef { + branchProtectionRule { + requiredStatusCheckContexts + requiredStatusChecks { + context + } + } + } statusCheckRollup { contexts(last: 100) { nodes { @@ -110,6 +118,7 @@ class GitHubService: ObservableObject { name status conclusion + isRequired(pullRequestNumber: \(req.number)) detailsUrl } ... on StatusContext { @@ -379,6 +388,7 @@ class GitHubService: ObservableObject { from: detail?.statusCheckRollup, mergeable: detail?.mergeable, mergeStateStatus: detail?.mergeStateStatus, + requiredStatusCheckContexts: detail?.requiredStatusCheckContexts, updatedAt: updatedAt, enableInactiveDetection: enableInactiveDetection, inactiveThresholdDays: inactiveThresholdDays @@ -387,7 +397,10 @@ class GitHubService: ObservableObject { labels: result.labels.map { PullRequest.Label(id: $0.id, name: $0.name, color: $0.color) }, type: prType, isDraft: result.isDraft, - statusChecks: parseStatusChecks(from: detail?.statusCheckRollup), + statusChecks: parseStatusChecks( + from: detail?.statusCheckRollup, + requiredStatusCheckContexts: detail?.requiredStatusCheckContexts + ), reviewDecision: GitHubService.resolveReviewDecision( rawValue: detail?.reviewDecision, latestReviews: detail?.latestReviews, @@ -399,29 +412,21 @@ class GitHubService: ObservableObject { } func fetchPRStatus(owner: String, repo: String, number: Int, updatedAt: Date, enableInactiveDetection: Bool, inactiveThresholdDays: Int, host: String = "github.com") async throws -> (status: BuildStatus, headRefName: String, statusChecks: [StatusCheck], reviewDecision: ReviewDecision?) { - let json = try await shellExecutor.execute( - command: "gh", - arguments: [ - "pr", "view", "\(number)", - "--repo", "\(owner)/\(repo)", - "--json", "headRefName,statusCheckRollup,mergeable,mergeStateStatus,reviewDecision,latestReviews,reviewRequests" - ], - host: host - ) - - guard let jsonData = json.data(using: .utf8) else { + let request = PRStatusRequest(owner: owner, repo: repo, number: number) + guard let detail = try await batchFetchStatuses(for: [request], host: host)[request] else { throw GitHubError.invalidResponse } - let decoder = JSONDecoder() - let detail = try decoder.decode(GHPRDetailResponse.self, from: jsonData) - - let statusChecks = parseStatusChecks(from: detail.statusCheckRollup) + let statusChecks = parseStatusChecks( + from: detail.statusCheckRollup, + requiredStatusCheckContexts: detail.requiredStatusCheckContexts + ) let status = parseOverallStatus( from: detail.statusCheckRollup, mergeable: detail.mergeable, mergeStateStatus: detail.mergeStateStatus, + requiredStatusCheckContexts: detail.requiredStatusCheckContexts, updatedAt: updatedAt, enableInactiveDetection: enableInactiveDetection, inactiveThresholdDays: inactiveThresholdDays @@ -472,7 +477,7 @@ class GitHubService: ObservableObject { return .changesRequested } - private func parseOverallStatus(from checks: [GHPRDetailResponse.StatusCheck]?, mergeable: String?, mergeStateStatus: String?, updatedAt: Date, enableInactiveDetection: Bool, inactiveThresholdDays: Int) -> BuildStatus { + private func parseOverallStatus(from checks: [GHPRDetailResponse.StatusCheck]?, mergeable: String?, mergeStateStatus: String?, requiredStatusCheckContexts: [String]? = nil, updatedAt: Date, enableInactiveDetection: Bool, inactiveThresholdDays: Int) -> BuildStatus { // Check for merge conflicts first (highest priority) if let mergeable = mergeable?.uppercased(), mergeable == "CONFLICTING" { return .conflict @@ -482,17 +487,21 @@ class GitHubService: ObservableObject { return .conflict } - guard let checks = checks, !checks.isEmpty else { - return .success - } + let checks = checks ?? [] + + let missingRequiredContexts = missingRequiredStatusCheckContexts( + in: checks, + requiredStatusCheckContexts: requiredStatusCheckContexts + ) + let checksToEvaluate = statusChecksForOverallStatus(checks, requiredStatusCheckContexts: requiredStatusCheckContexts) // Priority: conflict > failure > error > pending > success var hasFailure = false var hasError = false - var hasPending = false + var hasPending = !missingRequiredContexts.isEmpty var hasSuccess = false - for check in checks { + for check in checksToEvaluate { // Check conclusion field (for completed checks) if let conclusion = check.conclusion?.uppercased() { switch conclusion { @@ -563,16 +572,70 @@ class GitHubService: ObservableObject { return .success } - private func parseStatusChecks(from checks: [GHPRDetailResponse.StatusCheck]?) -> [StatusCheck] { + private func statusChecksForOverallStatus( + _ checks: [GHPRDetailResponse.StatusCheck], + requiredStatusCheckContexts: [String]? + ) -> [GHPRDetailResponse.StatusCheck] { + let requiredContextSet = requiredStatusCheckContexts.map(Set.init) + let requiredChecks = checks.filter { check in + if let isRequired = check.isRequired { + return isRequired + } + guard let requiredContextSet, let checkName = check.name ?? check.context else { + return false + } + return requiredContextSet.contains(checkName) + } + + if !requiredChecks.isEmpty { + return requiredChecks + } + + let hasRequiredMetadata = requiredContextSet != nil || checks.contains { $0.isRequired != nil } + return hasRequiredMetadata ? [] : checks + } + + private func missingRequiredStatusCheckContexts( + in checks: [GHPRDetailResponse.StatusCheck], + requiredStatusCheckContexts: [String]? + ) -> Set { + guard let requiredStatusCheckContexts else { return [] } + let presentNames = Set(checks.compactMap { $0.name ?? $0.context }) + return Set(requiredStatusCheckContexts).subtracting(presentNames) + } + + private func parseStatusChecks( + from checks: [GHPRDetailResponse.StatusCheck]?, + requiredStatusCheckContexts: [String]? = nil + ) -> [StatusCheck] { guard let checks = checks else { return [] } + let requiredContextSet = requiredStatusCheckContexts.map(Set.init) + let approvalWorkflowNames = Set(checks.compactMap { check -> String? in + guard let checkName = check.name ?? check.context, + isApprovalGate(checkName) else { + return nil + } + return circleCIWorkflowName(from: checkName) + }) + let pendingApprovalWorkflowNames = Set(checks.compactMap { check -> String? in + guard let checkName = check.name ?? check.context, + isApprovalGate(checkName), + check.state?.uppercased() == "PENDING" else { + return nil + } + return circleCIWorkflowName(from: checkName) + }) + return checks.compactMap { check in // Extract check name from either name (CheckRun) or context (StatusContext) guard let checkName = check.name ?? check.context else { return nil } + let isApprovalGate = isApprovalGate(checkName) + let workflowName = circleCIWorkflowName(from: checkName) // Map status/state/conclusion to simplified CheckStatus enum let checkStatus: CheckStatus @@ -594,7 +657,7 @@ class GitHubService: ObservableObject { case "FAILURE", "ERROR": checkStatus = .failure case "PENDING", "EXPECTED": - checkStatus = .pending + checkStatus = isApprovalGate ? .waiting : .pending case "SUCCESS": checkStatus = .success default: @@ -602,7 +665,13 @@ class GitHubService: ObservableObject { } } else if let status = check.status?.uppercased() { switch status { - case "IN_PROGRESS", "QUEUED", "WAITING", "PENDING": + case "IN_PROGRESS": + checkStatus = .running + case "WAITING": + checkStatus = .waiting + case "QUEUED": + checkStatus = .queued + case "PENDING": checkStatus = .pending case "COMPLETED": // If completed but no conclusion, assume success @@ -620,16 +689,32 @@ class GitHubService: ObservableObject { // Generate stable ID let id = "\(check.__typename)-\(checkName)" + let isRequired = check.isRequired ?? requiredContextSet.map { $0.contains(checkName) } + let isNonBlockingWorkflow = approvalWorkflowNames.contains(checkName) + && !pendingApprovalWorkflowNames.contains(checkName) + let isNonBlocking = isRequired == false && (isApprovalGate || isNonBlockingWorkflow) return StatusCheck( id: id, name: checkName, status: checkStatus, - detailsUrl: detailsUrl + detailsUrl: detailsUrl, + isRequired: isRequired, + isNonBlocking: isNonBlocking ) } } + private func isApprovalGate(_ checkName: String) -> Bool { + checkName.lowercased().contains("/approve") + } + + private func circleCIWorkflowName(from checkName: String) -> String? { + guard checkName.hasPrefix("ci/circleci: ") else { return nil } + let name = checkName.dropFirst("ci/circleci: ".count) + return name.split(separator: "/", maxSplits: 1).first.map(String.init) + } + private func parseDate(_ dateString: String) throws -> Date { let formatters: [ISO8601DateFormatter] = [ { @@ -676,15 +761,20 @@ class GitHubService: ObservableObject { /// Fetches a single Other PR by its identifier. /// Returns `nil` if the PR is closed/merged, not found, or inaccessible. func fetchOtherPR(_ id: OtherPRIdentifier, enableInactiveDetection: Bool, inactiveThresholdDays: Int) async -> PullRequest? { + let host = id.host + let owner = id.owner + let repo = id.repo + let number = id.number + do { let json = try await shellExecutor.execute( command: "gh", arguments: [ - "pr", "view", "\(id.number)", - "--repo", "\(id.owner)/\(id.repo)", + "pr", "view", "\(number)", + "--repo", "\(owner)/\(repo)", "--json", "number,title,url,author,updatedAt,labels,isDraft,headRefName,statusCheckRollup,mergeable,mergeStateStatus,reviewDecision,latestReviews,reviewRequests,state" ], - host: id.host + host: host ) guard let jsonData = json.data(using: .utf8) else { return nil } @@ -695,11 +785,18 @@ class GitHubService: ObservableObject { guard response.state.uppercased() == "OPEN" else { return nil } let updatedAt = try parseDate(response.updatedAt) - let statusChecks = parseStatusChecks(from: response.statusCheckRollup) + let request = PRStatusRequest(owner: owner, repo: repo, number: number) + let statusDetail = try await batchFetchStatuses(for: [request], host: host)[request] + let statusCheckRollup = statusDetail?.statusCheckRollup ?? response.statusCheckRollup + let statusChecks = parseStatusChecks( + from: statusCheckRollup, + requiredStatusCheckContexts: statusDetail?.requiredStatusCheckContexts + ) let status = parseOverallStatus( - from: response.statusCheckRollup, - mergeable: response.mergeable, - mergeStateStatus: response.mergeStateStatus, + from: statusCheckRollup, + mergeable: statusDetail?.mergeable ?? response.mergeable, + mergeStateStatus: statusDetail?.mergeStateStatus ?? response.mergeStateStatus, + requiredStatusCheckContexts: statusDetail?.requiredStatusCheckContexts, updatedAt: updatedAt, enableInactiveDetection: enableInactiveDetection, inactiveThresholdDays: inactiveThresholdDays @@ -714,8 +811,8 @@ class GitHubService: ObservableObject { number: response.number, title: response.title, repository: PullRequest.RepositoryInfo( - name: id.repo, - nameWithOwner: "\(id.owner)/\(id.repo)" + name: repo, + nameWithOwner: "\(owner)/\(repo)" ), url: response.url, author: PullRequest.Author(login: response.author.login), @@ -730,7 +827,7 @@ class GitHubService: ObservableObject { isDraft: response.isDraft, statusChecks: statusChecks, reviewDecision: reviewDecision, - host: id.host + host: host ) } catch { print("Error fetching Other PR \(id.owner)/\(id.repo)#\(id.number): \(error)") diff --git a/MonitorLizard/Views/PRRowView.swift b/MonitorLizard/Views/PRRowView.swift index 3b75e90..4245a28 100644 --- a/MonitorLizard/Views/PRRowView.swift +++ b/MonitorLizard/Views/PRRowView.swift @@ -10,6 +10,19 @@ extension ReviewDecision { } } +extension NonBlockingCheckState { + var color: Color { + switch self { + case .failed, .waitingForApproval: + return .orange.opacity(0.85) + case .running, .queued, .pending: + return .blue.opacity(0.85) + case .passed: + return .green.opacity(0.85) + } + } +} + struct PRRowView: View { let pr: PullRequest @EnvironmentObject var viewModel: PRMonitorViewModel @@ -47,7 +60,7 @@ struct PRRowView: View { private var failingChecks: [StatusCheck] { pr.statusChecks.filter { check in - check.status == .failure || check.status == .error + !check.isNonBlocking && (check.status == .failure || check.status == .error) } } @@ -220,6 +233,22 @@ struct PRRowView: View { .foregroundColor(.secondary) } + if let summary = pr.nonBlockingCheckSummary { + HStack(spacing: 4) { + Text("Non-blocking:") + .font(.caption2) + .foregroundStyle(.secondary) + + ForEach(Array(summary.segments.enumerated()), id: \.element.id) { index, segment in + Text(segment.text + (index == summary.segments.count - 1 ? "" : ",")) + .font(.caption2) + .foregroundStyle(segment.state.color) + } + } + .lineLimit(1) + .help("These checks do not block merging.") + } + // Labels if !pr.labels.isEmpty { HStack(spacing: 4) { diff --git a/MonitorLizardTests/GitHubServiceTests.swift b/MonitorLizardTests/GitHubServiceTests.swift index 6987d35..ec3a516 100644 --- a/MonitorLizardTests/GitHubServiceTests.swift +++ b/MonitorLizardTests/GitHubServiceTests.swift @@ -2,6 +2,86 @@ import Testing import Foundation @testable import MonitorLizard +// MARK: - Non-blocking Check Summary Tests + +@MainActor +struct NonBlockingCheckSummaryTests { + + private func makePR(statusChecks: [StatusCheck]) -> PullRequest { + PullRequest( + number: 1, + title: "Test PR", + repository: PullRequest.RepositoryInfo(name: "repo", nameWithOwner: "owner/repo"), + url: "https://github.com/owner/repo/pull/1", + author: PullRequest.Author(login: "author"), + headRefName: "feature/test", + updatedAt: Date(), + buildStatus: .success, + isWatched: false, + labels: [], + type: .authored, + isDraft: false, + statusChecks: statusChecks, + reviewDecision: nil, + host: "github.com" + ) + } + + @Test func summaryIsNilWhenOnlyRequiredChecksExist() { + let pr = makePR(statusChecks: [ + StatusCheck(id: "required", name: "required", status: .success, detailsUrl: nil, isRequired: true) + ]) + + #expect(pr.nonBlockingCheckSummary == nil) + } + + @Test func summaryIsNilWhenRequirednessIsUnknown() { + let pr = makePR(statusChecks: [ + StatusCheck(id: "unknown", name: "unknown", status: .pending, detailsUrl: nil, isRequired: nil) + ]) + + #expect(pr.nonBlockingCheckSummary == nil) + } + + @Test func summaryIsNilWhenAllNonBlockingChecksPassed() { + let pr = makePR(statusChecks: [ + StatusCheck(id: "optional", name: "optional", status: .success, detailsUrl: nil, isRequired: false, isNonBlocking: true) + ]) + + #expect(pr.nonBlockingCheckSummary == nil) + } + + @Test func summaryDescribesWaitingAndRunningNonBlockingChecks() throws { + let pr = makePR(statusChecks: [ + StatusCheck(id: "approval", name: "approval", status: .waiting, detailsUrl: nil, isRequired: false, isNonBlocking: true), + StatusCheck(id: "analysis", name: "analysis", status: .running, detailsUrl: nil, isRequired: false, isNonBlocking: true) + ]) + + let summary = try #require(pr.nonBlockingCheckSummary) + + #expect(summary.segments.map(\.text) == ["1 waiting for approval", "1 running"]) + } + + @Test func summaryDescribesFailedAndPassedNonBlockingChecksWhenAttentionIsNeeded() throws { + let pr = makePR(statusChecks: [ + StatusCheck(id: "optional-failed", name: "optional-failed", status: .failure, detailsUrl: nil, isRequired: false, isNonBlocking: true), + StatusCheck(id: "optional-passed", name: "optional-passed", status: .success, detailsUrl: nil, isRequired: false, isNonBlocking: true) + ]) + + let summary = try #require(pr.nonBlockingCheckSummary) + + #expect(summary.segments.map(\.text) == ["1 failed", "1 passed"]) + } + + @Test func summaryIgnoresNonRequiredChecksThatAreNotNonBlocking() { + let pr = makePR(statusChecks: [ + StatusCheck(id: "required-workflow-job", name: "required-workflow-job", status: .running, detailsUrl: nil, isRequired: false) + ]) + + #expect(pr.nonBlockingCheckSummary == nil) + } +} + // MARK: - Mock private actor MockShellExecutor: ShellExecuting { @@ -157,6 +237,18 @@ struct GitHubServiceBatchQueryTests { #expect(query.contains("reviewRequests")) } + @Test func buildBatchQueryIncludesRequiredCheckMetadata() { + let query = GitHubService.buildBatchQuery(for: [ + PRStatusRequest(owner: "alice", repo: "repo", number: 42) + ]) + + #expect(query.contains("isRequired(pullRequestNumber: 42)")) + #expect(query.contains("baseRef")) + #expect(query.contains("branchProtectionRule")) + #expect(query.contains("requiredStatusCheckContexts")) + #expect(query.contains("requiredStatusChecks")) + } + @Test func buildBatchQueryForEmptyListProducesValidQuery() { let query = GitHubService.buildBatchQuery(for: []) #expect(query.contains("query")) @@ -363,6 +455,154 @@ struct GitHubServiceBatchIntegrationTests { } """ + private static let requiredSuccessOptionalPendingResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "required_ci", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://ci.example.com/required", "context": null, "state": null, "targetUrl": null }, + { "__typename": "CheckRun", "name": "manual_approval", "status": "WAITING", "conclusion": null, "isRequired": false, "detailsUrl": "https://ci.example.com/manual", "context": null, "state": null, "targetUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["required_ci"], + "requiredStatusChecks": [{ "context": "required_ci" }] + } + } + } + } + } + } + """ + + private static let requiredSuccessOptionalFailureResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "required_ci", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://ci.example.com/required", "context": null, "state": null, "targetUrl": null }, + { "__typename": "CheckRun", "name": "manual_approval", "status": "COMPLETED", "conclusion": "FAILURE", "isRequired": false, "detailsUrl": "https://ci.example.com/manual", "context": null, "state": null, "targetUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["required_ci"], + "requiredStatusChecks": [{ "context": "required_ci" }] + } + } + } + } + } + } + """ + + private static let missingRequiredMetadataPendingResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "unknown_ci", "status": "WAITING", "conclusion": null, "detailsUrl": "https://ci.example.com/unknown", "context": null, "state": null, "targetUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { "branchProtectionRule": null } + } + } + } + } + """ + + private static let requiredContextMissingApprovalWaitingResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "version_health / assessment", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://github.com/example/version-health", "context": null, "state": null, "targetUrl": null }, + { "__typename": "CheckRun", "name": "dead_code_cleanup", "status": "IN_PROGRESS", "conclusion": null, "isRequired": false, "detailsUrl": "https://app.circleci.com/pipelines/gh/owner/repo/1/workflows/optional", "context": null, "state": null, "targetUrl": null }, + { "__typename": "CheckRun", "name": "pull_requests", "status": "IN_PROGRESS", "conclusion": null, "isRequired": false, "detailsUrl": "https://app.circleci.com/pipelines/gh/owner/repo/1/workflows/required", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "context": "ci/circleci: dead_code_cleanup/approve_dead_code_cleanup", "state": "PENDING", "targetUrl": "https://circleci.com/workflow-run/optional", "detailsUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "context": "ci/circleci: run_unit_tests", "state": "PENDING", "targetUrl": "https://circleci.com/gh/owner/repo/100", "detailsUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "context": "ci/circleci: check_circleci_config_lint", "state": "SUCCESS", "targetUrl": "https://circleci.com/gh/owner/repo/101", "detailsUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "BLOCKED", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["ci/circleci: required_jobs_met", "version_health / assessment"], + "requiredStatusChecks": [{ "context": "ci/circleci: required_jobs_met" }, { "context": "version_health / assessment" }] + } + } + } + } + } + } + """ + + private static let emptyRollupRequiredContextResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "contexts": { "nodes": [] } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["ci/circleci: required_jobs_met"], + "requiredStatusChecks": [{ "context": "ci/circleci: required_jobs_met" }] + } + } + } + } + } + } + """ + @Test func fetchAllOpenPRsUsesBatchGraphQLInsteadOfPerPRView() async throws { let mock = MockShellExecutor( executeResponseMatchers: [ @@ -397,4 +637,76 @@ struct GitHubServiceBatchIntegrationTests { #expect(result.pullRequests.first?.headRefName == "feature/test") } + + @Test func fetchAllOpenPRsTreatsOptionalPendingChecksAsSuccessWhenRequiredChecksPass() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.requiredSuccessOptionalPendingResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + + #expect(result.pullRequests.first?.buildStatus == .success) + } + + @Test func fetchAllOpenPRsTreatsOptionalFailingChecksAsSuccessWhenRequiredChecksPass() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.requiredSuccessOptionalFailureResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + + #expect(result.pullRequests.first?.buildStatus == .success) + } + + @Test func fetchAllOpenPRsKeepsPendingStatusWhenRequiredMetadataIsUnknown() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.missingRequiredMetadataPendingResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + + #expect(result.pullRequests.first?.buildStatus == .pending) + } + + @Test func fetchAllOpenPRsTreatsMissingRequiredContextAsPendingAndSummarizesApprovalGate() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.requiredContextMissingApprovalWaitingResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + let pr = try #require(result.pullRequests.first) + + #expect(pr.buildStatus == .pending) + #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 waiting for approval"]) + } + + @Test func fetchAllOpenPRsTreatsEmptyRollupWithRequiredContextAsPending() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.emptyRollupRequiredContextResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + + #expect(result.pullRequests.first?.buildStatus == .pending) + } } From c3db3b89faa3868ae18fe8e19460ece700ed9c70 Mon Sep 17 00:00:00 2001 From: Luke LaBonte Date: Mon, 11 May 2026 12:22:36 -0500 Subject: [PATCH 2/4] Refine required CI status handling --- MonitorLizard/Models/BuildStatus.swift | 16 +- MonitorLizard/Models/PullRequest.swift | 57 ++- MonitorLizard/Services/GitHubService.swift | 261 +++++++++--- MonitorLizard/Views/PRRowView.swift | 14 +- MonitorLizardTests/GitHubServiceTests.swift | 395 +++++++++++++++++- .../PRMonitorViewModelTests.swift | 4 + 6 files changed, 647 insertions(+), 100 deletions(-) diff --git a/MonitorLizard/Models/BuildStatus.swift b/MonitorLizard/Models/BuildStatus.swift index aec02a7..11021f2 100644 --- a/MonitorLizard/Models/BuildStatus.swift +++ b/MonitorLizard/Models/BuildStatus.swift @@ -1,7 +1,8 @@ import SwiftUI -enum BuildStatus: String, Codable, Hashable { +enum BuildStatus: String, Codable, Hashable, CaseIterable { case conflict + case notStarted case pending case success case failure @@ -12,6 +13,7 @@ enum BuildStatus: String, Codable, Hashable { var icon: String { switch self { case .conflict: return "❗" + case .notStarted: return "🛑" case .success: return "✅" case .failure: return "❌" case .error: return "⚠️" @@ -21,9 +23,20 @@ enum BuildStatus: String, Codable, Hashable { } } + var systemImageName: String? { + switch self { + case .notStarted: return "play.slash" + case .pending: return "gear" + case .success: return "gear.badge.checkmark" + case .failure, .error: return "gear.badge.xmark" + case .conflict, .unknown, .inactive: return nil + } + } + var color: Color { switch self { case .conflict: return .purple + case .notStarted: return .gray case .success: return .green case .failure: return .red case .error: return .orange @@ -36,6 +49,7 @@ enum BuildStatus: String, Codable, Hashable { var displayName: String { switch self { case .conflict: return "Merge Conflict" + case .notStarted: return "Not started" case .success: return "Success" case .failure: return "Failed" case .error: return "Error" diff --git a/MonitorLizard/Models/PullRequest.swift b/MonitorLizard/Models/PullRequest.swift index f48148f..180fb02 100644 --- a/MonitorLizard/Models/PullRequest.swift +++ b/MonitorLizard/Models/PullRequest.swift @@ -150,7 +150,6 @@ extension PullRequest { .running, .queued, .pending, - .passed, ].compactMap { state -> NonBlockingCheckSummary.Segment? in guard let count = counts[state], count > 0 else { return nil } return NonBlockingCheckSummary.Segment(state: state, count: count) @@ -213,39 +212,18 @@ struct GHPRSearchResponse: Codable { } } -/// Combined response for `gh pr view --json number,title,url,author,updatedAt,labels,isDraft,headRefName,statusCheckRollup,mergeable,mergeStateStatus,reviewDecision,latestReviews,reviewRequests,state` -struct GHPRViewResponse: Codable { - let number: Int - let title: String - let url: String - let author: Author - let updatedAt: String - let labels: [Label] - let isDraft: Bool - let headRefName: String - let statusCheckRollup: [GHPRDetailResponse.StatusCheck]? - let mergeable: String? - let mergeStateStatus: String? - let reviewDecision: String? - let latestReviews: [GHPRDetailResponse.Review]? - let reviewRequests: [GHPRDetailResponse.ReviewRequest]? - let state: String - - struct Author: Codable { - let login: String - } - - struct Label: Codable { - let id: String? - let name: String - let color: String - } -} - /// Response structure for a single PR node inside a `gh api graphql` batch query. /// Unlike GHPRDetailResponse (used with `gh pr view --json`), review connections use /// `{ nodes: [...] }` format as returned by the raw GraphQL API. struct BatchPRStatusResponse: Codable { + let number: Int? + let title: String? + let url: String? + let author: Author? + let updatedAt: String? + let labels: LabelConnection? + let isDraft: Bool? + let state: String? let headRefName: String let statusCheckRollup: StatusCheckRollupWrapper? let mergeable: String? @@ -257,6 +235,7 @@ struct BatchPRStatusResponse: Codable { /// Wraps the raw GraphQL `statusCheckRollup { contexts { nodes [...] } }` shape. struct StatusCheckRollupWrapper: Codable { + let state: String? let contexts: Contexts? struct Contexts: Codable { @@ -264,6 +243,20 @@ struct BatchPRStatusResponse: Codable { } } + struct Author: Codable { + let login: String + } + + struct LabelConnection: Codable { + let nodes: [Label]? + + struct Label: Codable { + let id: String + let name: String + let color: String + } + } + struct ReviewConnection: Codable { let nodes: [GHPRDetailResponse.Review]? } @@ -308,6 +301,7 @@ struct BatchPRStatusResponse: Codable { return GHPRDetailResponse( headRefName: headRefName, statusCheckRollup: statusCheckRollup?.contexts?.nodes, + statusCheckRollupState: statusCheckRollup?.state, mergeable: mergeable, mergeStateStatus: mergeStateStatus, reviewDecision: reviewDecision, @@ -331,6 +325,7 @@ struct BatchGraphQLResponse: Codable { struct GHPRDetailResponse: Codable { let headRefName: String let statusCheckRollup: [StatusCheck]? + let statusCheckRollupState: String? let mergeable: String? let mergeStateStatus: String? let reviewDecision: String? @@ -341,6 +336,7 @@ struct GHPRDetailResponse: Codable { init( headRefName: String, statusCheckRollup: [StatusCheck]?, + statusCheckRollupState: String? = nil, mergeable: String?, mergeStateStatus: String?, reviewDecision: String?, @@ -350,6 +346,7 @@ struct GHPRDetailResponse: Codable { ) { self.headRefName = headRefName self.statusCheckRollup = statusCheckRollup + self.statusCheckRollupState = statusCheckRollupState self.mergeable = mergeable self.mergeStateStatus = mergeStateStatus self.reviewDecision = reviewDecision diff --git a/MonitorLizard/Services/GitHubService.swift b/MonitorLizard/Services/GitHubService.swift index 0fee04b..d80558c 100644 --- a/MonitorLizard/Services/GitHubService.swift +++ b/MonitorLizard/Services/GitHubService.swift @@ -111,6 +111,7 @@ class GitHubService: ObservableObject { } } statusCheckRollup { + state contexts(last: 100) { nodes { ... on CheckRun { @@ -125,6 +126,7 @@ class GitHubService: ObservableObject { __typename context state + isRequired(pullRequestNumber: \(req.number)) targetUrl } } @@ -154,6 +156,78 @@ class GitHubService: ObservableObject { return "query {\n\(fragments.joined(separator: "\n"))}" } + nonisolated static func buildPRDetailQuery(for request: PRStatusRequest) -> String { + """ + query { + pr0: repository(owner: "\(request.owner)", name: "\(request.repo)") { + pullRequest(number: \(request.number)) { + number + title + url + author { login } + updatedAt + labels(first: 20) { + nodes { + id + name + color + } + } + isDraft + state + headRefName + baseRef { + branchProtectionRule { + requiredStatusCheckContexts + requiredStatusChecks { + context + } + } + } + statusCheckRollup { + state + contexts(last: 100) { + nodes { + ... on CheckRun { + __typename + name + status + conclusion + isRequired(pullRequestNumber: \(request.number)) + detailsUrl + } + ... on StatusContext { + __typename + context + state + isRequired(pullRequestNumber: \(request.number)) + targetUrl + } + } + } + } + mergeable + mergeStateStatus + reviewDecision + latestReviews(last: 20) { + nodes { + state + author { login } + } + } + reviewRequests(last: 20) { + nodes { + requestedReviewer { + ... on User { login } + } + } + } + } + } + } + """ + } + /// Parses a `gh api graphql` batch response and maps the per-alias results back to /// the original `PRStatusRequest` values. PRs whose `pullRequest` field is null /// (closed, deleted, or inaccessible) are omitted from the returned dictionary. @@ -386,6 +460,7 @@ class GitHubService: ObservableObject { updatedAt: updatedAt, buildStatus: parseOverallStatus( from: detail?.statusCheckRollup, + statusCheckRollupState: detail?.statusCheckRollupState, mergeable: detail?.mergeable, mergeStateStatus: detail?.mergeStateStatus, requiredStatusCheckContexts: detail?.requiredStatusCheckContexts, @@ -399,6 +474,7 @@ class GitHubService: ObservableObject { isDraft: result.isDraft, statusChecks: parseStatusChecks( from: detail?.statusCheckRollup, + statusCheckRollupState: detail?.statusCheckRollupState, requiredStatusCheckContexts: detail?.requiredStatusCheckContexts ), reviewDecision: GitHubService.resolveReviewDecision( @@ -419,11 +495,13 @@ class GitHubService: ObservableObject { let statusChecks = parseStatusChecks( from: detail.statusCheckRollup, + statusCheckRollupState: detail.statusCheckRollupState, requiredStatusCheckContexts: detail.requiredStatusCheckContexts ) let status = parseOverallStatus( from: detail.statusCheckRollup, + statusCheckRollupState: detail.statusCheckRollupState, mergeable: detail.mergeable, mergeStateStatus: detail.mergeStateStatus, requiredStatusCheckContexts: detail.requiredStatusCheckContexts, @@ -477,7 +555,7 @@ class GitHubService: ObservableObject { return .changesRequested } - private func parseOverallStatus(from checks: [GHPRDetailResponse.StatusCheck]?, mergeable: String?, mergeStateStatus: String?, requiredStatusCheckContexts: [String]? = nil, updatedAt: Date, enableInactiveDetection: Bool, inactiveThresholdDays: Int) -> BuildStatus { + private func parseOverallStatus(from checks: [GHPRDetailResponse.StatusCheck]?, statusCheckRollupState: String? = nil, mergeable: String?, mergeStateStatus: String?, requiredStatusCheckContexts: [String]? = nil, updatedAt: Date, enableInactiveDetection: Bool, inactiveThresholdDays: Int) -> BuildStatus { // Check for merge conflicts first (highest priority) if let mergeable = mergeable?.uppercased(), mergeable == "CONFLICTING" { return .conflict @@ -501,6 +579,17 @@ class GitHubService: ObservableObject { var hasPending = !missingRequiredContexts.isEmpty var hasSuccess = false + if !missingRequiredContexts.isEmpty { + switch statusCheckRollupState?.uppercased() { + case "FAILURE": + hasFailure = true + case "ERROR": + hasError = true + default: + break + } + } + for check in checksToEvaluate { // Check conclusion field (for completed checks) if let conclusion = check.conclusion?.uppercased() { @@ -552,6 +641,10 @@ class GitHubService: ObservableObject { return .error } + if !missingRequiredContexts.isEmpty && !hasActiveNonApprovalWork(in: checks) { + return .notStarted + } + if hasPending { return .pending } @@ -572,6 +665,25 @@ class GitHubService: ObservableObject { return .success } + private func hasActiveNonApprovalWork(in checks: [GHPRDetailResponse.StatusCheck]) -> Bool { + checks.contains { check in + guard let checkName = check.name ?? check.context, + !isApprovalGate(checkName) else { + return false + } + + if let state = check.state?.uppercased(), ["PENDING", "EXPECTED"].contains(state) { + return true + } + + if let status = check.status?.uppercased(), ["IN_PROGRESS", "QUEUED", "WAITING", "PENDING"].contains(status) { + return true + } + + return false + } + } + private func statusChecksForOverallStatus( _ checks: [GHPRDetailResponse.StatusCheck], requiredStatusCheckContexts: [String]? @@ -606,6 +718,7 @@ class GitHubService: ObservableObject { private func parseStatusChecks( from checks: [GHPRDetailResponse.StatusCheck]?, + statusCheckRollupState: String? = nil, requiredStatusCheckContexts: [String]? = nil ) -> [StatusCheck] { guard let checks = checks else { @@ -613,20 +726,19 @@ class GitHubService: ObservableObject { } let requiredContextSet = requiredStatusCheckContexts.map(Set.init) - let approvalWorkflowNames = Set(checks.compactMap { check -> String? in - guard let checkName = check.name ?? check.context, - isApprovalGate(checkName) else { - return nil - } - return circleCIWorkflowName(from: checkName) - }) + let missingRequiredContexts = missingRequiredStatusCheckContexts( + in: checks, + requiredStatusCheckContexts: requiredStatusCheckContexts + ) + let rollupFailedWithMissingRequiredContext = !missingRequiredContexts.isEmpty + && ["FAILURE", "ERROR"].contains(statusCheckRollupState?.uppercased()) let pendingApprovalWorkflowNames = Set(checks.compactMap { check -> String? in guard let checkName = check.name ?? check.context, - isApprovalGate(checkName), - check.state?.uppercased() == "PENDING" else { + (check.isRequired ?? requiredContextSet.map { $0.contains(checkName) }) == false, + ["PENDING", "EXPECTED"].contains(check.state?.uppercased()) else { return nil } - return circleCIWorkflowName(from: checkName) + return parentWorkflowNameForApprovalGate(checkName) }) return checks.compactMap { check in @@ -634,8 +746,6 @@ class GitHubService: ObservableObject { guard let checkName = check.name ?? check.context else { return nil } - let isApprovalGate = isApprovalGate(checkName) - let workflowName = circleCIWorkflowName(from: checkName) // Map status/state/conclusion to simplified CheckStatus enum let checkStatus: CheckStatus @@ -657,7 +767,7 @@ class GitHubService: ObservableObject { case "FAILURE", "ERROR": checkStatus = .failure case "PENDING", "EXPECTED": - checkStatus = isApprovalGate ? .waiting : .pending + checkStatus = isApprovalGate(checkName) ? .waiting : .pending case "SUCCESS": checkStatus = .success default: @@ -690,9 +800,18 @@ class GitHubService: ObservableObject { // Generate stable ID let id = "\(check.__typename)-\(checkName)" let isRequired = check.isRequired ?? requiredContextSet.map { $0.contains(checkName) } - let isNonBlockingWorkflow = approvalWorkflowNames.contains(checkName) - && !pendingApprovalWorkflowNames.contains(checkName) - let isNonBlocking = isRequired == false && (isApprovalGate || isNonBlockingWorkflow) + let isBlockingFailure = rollupFailedWithMissingRequiredContext + && isRequired == false + && (checkStatus == .failure || checkStatus == .error) + let isWaitingForApprovalParent = check.__typename == "CheckRun" + && isRequired == false + && checkStatus == .running + && pendingApprovalWorkflowNames.contains(checkName) + let isRequiredAggregateWork = !missingRequiredContexts.isEmpty && !isApprovalGate(checkName) + let isNonBlocking = isRequired == false + && !isBlockingFailure + && !isWaitingForApprovalParent + && !isRequiredAggregateWork return StatusCheck( id: id, @@ -706,13 +825,33 @@ class GitHubService: ObservableObject { } private func isApprovalGate(_ checkName: String) -> Bool { - checkName.lowercased().contains("/approve") + guard let gateName = approvalGateName(checkName) else { return false } + let lowercasedName = gateName.lowercased() + return lowercasedName.hasPrefix("approve") || lowercasedName.contains("approval") } - private func circleCIWorkflowName(from checkName: String) -> String? { - guard checkName.hasPrefix("ci/circleci: ") else { return nil } - let name = checkName.dropFirst("ci/circleci: ".count) - return name.split(separator: "/", maxSplits: 1).first.map(String.init) + private func parentWorkflowNameForApprovalGate(_ checkName: String) -> String? { + guard isApprovalGate(checkName) else { return nil } + + let components = workflowPathComponents(checkName) + guard components.count == 2 else { return nil } + + let workflowName = components[0].trimmingCharacters(in: .whitespacesAndNewlines) + return workflowName.isEmpty ? nil : workflowName + } + + private func approvalGateName(_ checkName: String) -> String? { + let components = workflowPathComponents(checkName) + guard components.count == 2 else { return nil } + + let gateName = components[1].trimmingCharacters(in: .whitespacesAndNewlines) + return gateName.isEmpty ? nil : gateName + } + + private func workflowPathComponents(_ checkName: String) -> [Substring] { + let suffix = checkName.split(separator: ":", maxSplits: 1).last.map(String.init) ?? checkName + let path = suffix.trimmingCharacters(in: .whitespacesAndNewlines) + return path.split(separator: "/", maxSplits: 1) } private func parseDate(_ dateString: String) throws -> Date { @@ -766,65 +905,62 @@ class GitHubService: ObservableObject { let repo = id.repo let number = id.number + let request = PRStatusRequest(owner: owner, repo: repo, number: number) do { - let json = try await shellExecutor.execute( - command: "gh", - arguments: [ - "pr", "view", "\(number)", - "--repo", "\(owner)/\(repo)", - "--json", "number,title,url,author,updatedAt,labels,isDraft,headRefName,statusCheckRollup,mergeable,mergeStateStatus,reviewDecision,latestReviews,reviewRequests,state" - ], - host: host - ) - - guard let jsonData = json.data(using: .utf8) else { return nil } - - let decoder = JSONDecoder() - let response = try decoder.decode(GHPRViewResponse.self, from: jsonData) - - guard response.state.uppercased() == "OPEN" else { return nil } + guard let response = try await fetchPRDetail(for: request, host: host), + response.state?.uppercased() == "OPEN", + let responseNumber = response.number, + let title = response.title, + let url = response.url, + let author = response.author, + let updatedAtString = response.updatedAt, + let isDraft = response.isDraft else { + return nil + } - let updatedAt = try parseDate(response.updatedAt) - let request = PRStatusRequest(owner: owner, repo: repo, number: number) - let statusDetail = try await batchFetchStatuses(for: [request], host: host)[request] - let statusCheckRollup = statusDetail?.statusCheckRollup ?? response.statusCheckRollup + let updatedAt = try parseDate(updatedAtString) + let statusCheckRollup = response.statusCheckRollup?.contexts?.nodes let statusChecks = parseStatusChecks( from: statusCheckRollup, - requiredStatusCheckContexts: statusDetail?.requiredStatusCheckContexts + statusCheckRollupState: response.statusCheckRollup?.state, + requiredStatusCheckContexts: response.requiredStatusCheckContexts ) let status = parseOverallStatus( from: statusCheckRollup, - mergeable: statusDetail?.mergeable ?? response.mergeable, - mergeStateStatus: statusDetail?.mergeStateStatus ?? response.mergeStateStatus, - requiredStatusCheckContexts: statusDetail?.requiredStatusCheckContexts, + statusCheckRollupState: response.statusCheckRollup?.state, + mergeable: response.mergeable, + mergeStateStatus: response.mergeStateStatus, + requiredStatusCheckContexts: response.requiredStatusCheckContexts, updatedAt: updatedAt, enableInactiveDetection: enableInactiveDetection, inactiveThresholdDays: inactiveThresholdDays ) let reviewDecision = Self.resolveReviewDecision( rawValue: response.reviewDecision, - latestReviews: response.latestReviews, - reviewRequests: response.reviewRequests + latestReviews: response.latestReviews?.nodes, + reviewRequests: response.reviewRequests?.nodes?.map { + GHPRDetailResponse.ReviewRequest(login: $0.requestedReviewer?.login) + } ) return PullRequest( - number: response.number, - title: response.title, + number: responseNumber, + title: title, repository: PullRequest.RepositoryInfo( name: repo, nameWithOwner: "\(owner)/\(repo)" ), - url: response.url, - author: PullRequest.Author(login: response.author.login), + url: url, + author: PullRequest.Author(login: author.login), headRefName: response.headRefName, updatedAt: updatedAt, buildStatus: status, isWatched: false, - labels: response.labels.map { label in - PullRequest.Label(id: label.id ?? label.name, name: label.name, color: label.color) + labels: (response.labels?.nodes ?? []).map { label in + PullRequest.Label(id: label.id, name: label.name, color: label.color) }, type: .other, - isDraft: response.isDraft, + isDraft: isDraft, statusChecks: statusChecks, reviewDecision: reviewDecision, host: host @@ -835,6 +971,21 @@ class GitHubService: ObservableObject { } } + private func fetchPRDetail(for request: PRStatusRequest, host: String) async throws -> BatchPRStatusResponse? { + let query = GitHubService.buildPRDetailQuery(for: request) + let json = try await shellExecutor.execute( + command: "gh", + arguments: ["api", "graphql", "-f", "query=\(query)"], + host: host + ) + + guard let data = json.data(using: .utf8) else { + throw GitHubError.invalidResponse + } + let response = try JSONDecoder().decode(BatchGraphQLResponse.self, from: data) + return response.data["pr0"]?.pullRequest + } + private func extractOwner(from nameWithOwner: String) -> String { let components = nameWithOwner.split(separator: "/") return components.first.map(String.init) ?? "" diff --git a/MonitorLizard/Views/PRRowView.swift b/MonitorLizard/Views/PRRowView.swift index 4245a28..cda9aa5 100644 --- a/MonitorLizard/Views/PRRowView.swift +++ b/MonitorLizard/Views/PRRowView.swift @@ -124,13 +124,9 @@ struct PRRowView: View { ProgressView() .scaleEffect(0.8) } - } else if pr.buildStatus == .success { - Image(systemName: "gear.badge.checkmark") - .foregroundColor(.green) - .font(.title2) - } else if pr.buildStatus == .failure || pr.buildStatus == .error { - Image(systemName: "gear.badge.xmark") - .foregroundColor(.red) + } else if let systemImageName = pr.buildStatus.systemImageName { + Image(systemName: systemImageName) + .foregroundColor(pr.buildStatus.color) .font(.title2) } else { Text(pr.buildStatus.icon) @@ -268,6 +264,10 @@ struct PRRowView: View { // Failing checks (only shown when checks fail) if !failingChecks.isEmpty { VStack(alignment: .leading, spacing: 2) { + Text("Failed checks:") + .font(.caption2) + .foregroundStyle(.secondary) + ForEach(failingChecks) { check in Button(action: { openCheckURL(check.detailsUrl) diff --git a/MonitorLizardTests/GitHubServiceTests.swift b/MonitorLizardTests/GitHubServiceTests.swift index ec3a516..cb87615 100644 --- a/MonitorLizardTests/GitHubServiceTests.swift +++ b/MonitorLizardTests/GitHubServiceTests.swift @@ -2,6 +2,32 @@ import Testing import Foundation @testable import MonitorLizard +// MARK: - Build Status Presentation Tests + +@MainActor +struct BuildStatusPresentationTests { + + @Test func buildStatusPresentationCoversEveryState() { + let presentations: [(status: BuildStatus, displayName: String, icon: String, systemImageName: String?)] = [ + (.conflict, "Merge Conflict", "❗", nil), + (.notStarted, "Not started", "🛑", "play.slash"), + (.pending, "Pending", "🔄", "gear"), + (.success, "Success", "✅", "gear.badge.checkmark"), + (.failure, "Failed", "❌", "gear.badge.xmark"), + (.error, "Error", "⚠️", "gear.badge.xmark"), + (.unknown, "Unknown", "❓", nil), + (.inactive, "Inactive", "⏳", nil), + ] + + #expect(presentations.map(\.status) == BuildStatus.allCases) + for presentation in presentations { + #expect(presentation.status.displayName == presentation.displayName) + #expect(presentation.status.icon == presentation.icon) + #expect(presentation.status.systemImageName == presentation.systemImageName) + } + } +} + // MARK: - Non-blocking Check Summary Tests @MainActor @@ -62,7 +88,24 @@ struct NonBlockingCheckSummaryTests { #expect(summary.segments.map(\.text) == ["1 waiting for approval", "1 running"]) } - @Test func summaryDescribesFailedAndPassedNonBlockingChecksWhenAttentionIsNeeded() throws { + @Test func summaryOrdersAllActionableStatesAndOmitsPassedStates() throws { + let pr = makePR(statusChecks: [ + StatusCheck(id: "optional-failed", name: "optional-failed", status: .failure, detailsUrl: nil, isRequired: false, isNonBlocking: true), + StatusCheck(id: "optional-error", name: "optional-error", status: .error, detailsUrl: nil, isRequired: false, isNonBlocking: true), + StatusCheck(id: "optional-waiting", name: "optional-waiting", status: .waiting, detailsUrl: nil, isRequired: false, isNonBlocking: true), + StatusCheck(id: "optional-running", name: "optional-running", status: .running, detailsUrl: nil, isRequired: false, isNonBlocking: true), + StatusCheck(id: "optional-queued", name: "optional-queued", status: .queued, detailsUrl: nil, isRequired: false, isNonBlocking: true), + StatusCheck(id: "optional-pending", name: "optional-pending", status: .pending, detailsUrl: nil, isRequired: false, isNonBlocking: true), + StatusCheck(id: "optional-success", name: "optional-success", status: .success, detailsUrl: nil, isRequired: false, isNonBlocking: true), + StatusCheck(id: "optional-skipped", name: "optional-skipped", status: .skipped, detailsUrl: nil, isRequired: false, isNonBlocking: true), + ]) + + let summary = try #require(pr.nonBlockingCheckSummary) + + #expect(summary.segments.map(\.text) == ["2 failed", "1 waiting for approval", "1 running", "1 queued", "1 pending"]) + } + + @Test func summaryOmitsPassedNonBlockingChecksWhenAttentionIsNeeded() throws { let pr = makePR(statusChecks: [ StatusCheck(id: "optional-failed", name: "optional-failed", status: .failure, detailsUrl: nil, isRequired: false, isNonBlocking: true), StatusCheck(id: "optional-passed", name: "optional-passed", status: .success, detailsUrl: nil, isRequired: false, isNonBlocking: true) @@ -70,7 +113,7 @@ struct NonBlockingCheckSummaryTests { let summary = try #require(pr.nonBlockingCheckSummary) - #expect(summary.segments.map(\.text) == ["1 failed", "1 passed"]) + #expect(summary.segments.map(\.text) == ["1 failed"]) } @Test func summaryIgnoresNonRequiredChecksThatAreNotNonBlocking() { @@ -230,6 +273,7 @@ struct GitHubServiceBatchQueryTests { ]) #expect(query.contains("headRefName")) #expect(query.contains("statusCheckRollup")) + #expect(query.range(of: #"statusCheckRollup\s*\{\s*state"#, options: .regularExpression) != nil) #expect(query.contains("mergeable")) #expect(query.contains("mergeStateStatus")) #expect(query.contains("reviewDecision")) @@ -243,10 +287,24 @@ struct GitHubServiceBatchQueryTests { ]) #expect(query.contains("isRequired(pullRequestNumber: 42)")) + #expect(query.components(separatedBy: "isRequired(pullRequestNumber: 42)").count - 1 == 2) #expect(query.contains("baseRef")) #expect(query.contains("branchProtectionRule")) #expect(query.contains("requiredStatusCheckContexts")) #expect(query.contains("requiredStatusChecks")) + #expect(query.range(of: #"statusCheckRollup\s*\{\s*state"#, options: .regularExpression) != nil) + } + + @Test func buildPRDetailQueryIncludesRequiredCheckMetadata() { + let query = GitHubService.buildPRDetailQuery(for: PRStatusRequest(owner: "alice", repo: "repo", number: 42)) + + #expect(query.contains("isRequired(pullRequestNumber: 42)")) + #expect(query.components(separatedBy: "isRequired(pullRequestNumber: 42)").count - 1 == 2) + #expect(query.contains("baseRef")) + #expect(query.contains("branchProtectionRule")) + #expect(query.contains("requiredStatusCheckContexts")) + #expect(query.contains("requiredStatusChecks")) + #expect(query.range(of: #"statusCheckRollup\s*\{\s*state"#, options: .regularExpression) != nil) } @Test func buildBatchQueryForEmptyListProducesValidQuery() { @@ -517,6 +575,69 @@ struct GitHubServiceBatchIntegrationTests { } """ + private static let optionalApprovalNamedStatusContextResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "required_ci", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://ci.example.com/required", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: approval_tests", "state": "PENDING", "targetUrl": "https://ci.example.com/approval-tests", "detailsUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["required_ci"], + "requiredStatusChecks": [{ "context": "required_ci" }] + } + } + } + } + } + } + """ + + private static let requiredApprovalGateDoesNotSuppressOptionalParentResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "required_ci", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://ci.example.com/required", "context": null, "state": null, "targetUrl": null }, + { "__typename": "CheckRun", "name": "deploy", "status": "IN_PROGRESS", "conclusion": null, "isRequired": false, "detailsUrl": "https://ci.example.com/deploy", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": true, "context": "ci/example: deploy/approve_deploy", "state": "PENDING", "targetUrl": "https://ci.example.com/deploy/approve", "detailsUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "BLOCKED", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["required_ci", "ci/example: deploy/approve_deploy"], + "requiredStatusChecks": [{ "context": "required_ci" }, { "context": "ci/example: deploy/approve_deploy" }] + } + } + } + } + } + } + """ + private static let missingRequiredMetadataPendingResult = """ { "data": { @@ -549,13 +670,15 @@ struct GitHubServiceBatchIntegrationTests { "pullRequest": { "headRefName": "feature/test", "statusCheckRollup": { + "state": "FAILURE", "contexts": { "nodes": [ { "__typename": "CheckRun", "name": "version_health / assessment", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://github.com/example/version-health", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/circleci: preflight_check", "state": "FAILURE", "targetUrl": "https://circleci.com/gh/owner/repo/105140", "detailsUrl": null }, + { "__typename": "CheckRun", "name": "pull_requests", "status": "COMPLETED", "conclusion": "FAILURE", "isRequired": false, "detailsUrl": "https://app.circleci.com/pipelines/gh/owner/repo/1/workflows/required", "context": null, "state": null, "targetUrl": null }, { "__typename": "CheckRun", "name": "dead_code_cleanup", "status": "IN_PROGRESS", "conclusion": null, "isRequired": false, "detailsUrl": "https://app.circleci.com/pipelines/gh/owner/repo/1/workflows/optional", "context": null, "state": null, "targetUrl": null }, - { "__typename": "CheckRun", "name": "pull_requests", "status": "IN_PROGRESS", "conclusion": null, "isRequired": false, "detailsUrl": "https://app.circleci.com/pipelines/gh/owner/repo/1/workflows/required", "context": null, "state": null, "targetUrl": null }, { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "context": "ci/circleci: dead_code_cleanup/approve_dead_code_cleanup", "state": "PENDING", "targetUrl": "https://circleci.com/workflow-run/optional", "detailsUrl": null }, - { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "context": "ci/circleci: run_unit_tests", "state": "PENDING", "targetUrl": "https://circleci.com/gh/owner/repo/100", "detailsUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "context": "ci/circleci: check_mobsfscan", "state": "SUCCESS", "targetUrl": "https://circleci.com/gh/owner/repo/101", "detailsUrl": null }, { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "context": "ci/circleci: check_circleci_config_lint", "state": "SUCCESS", "targetUrl": "https://circleci.com/gh/owner/repo/101", "detailsUrl": null } ] } @@ -577,6 +700,77 @@ struct GitHubServiceBatchIntegrationTests { } """ + private static let missingRequiredContextNotStartedResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "state": "SUCCESS", + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "version_health / assessment", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://github.com/example/version-health", "context": null, "state": null, "targetUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "BLOCKED", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["ci/example: required_jobs_met", "version_health / assessment"], + "requiredStatusChecks": [{ "context": "ci/example: required_jobs_met" }, { "context": "version_health / assessment" }] + } + } + } + } + } + } + """ + + private static let missingRequiredContextWithRequiredWorkflowProgressResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "state": "PENDING", + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "version_health / assessment", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://github.com/example/version-health", "context": null, "state": null, "targetUrl": null }, + { "__typename": "CheckRun", "name": "dead_code_cleanup", "status": "IN_PROGRESS", "conclusion": null, "isRequired": false, "detailsUrl": "https://ci.example.com/workflows/optional", "context": null, "state": null, "targetUrl": null }, + { "__typename": "CheckRun", "name": "pull_requests", "status": "IN_PROGRESS", "conclusion": null, "isRequired": false, "detailsUrl": "https://ci.example.com/workflows/required", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: dead_code_cleanup/approve_dead_code_cleanup", "state": "PENDING", "targetUrl": "https://ci.example.com/workflows/optional/approve", "detailsUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: generate_beta_build", "state": "PENDING", "targetUrl": "https://ci.example.com/jobs/generate_beta_build", "detailsUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: generate_release_build", "state": "PENDING", "targetUrl": "https://ci.example.com/jobs/generate_release_build", "detailsUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: generate_simulator_debug_build", "state": "PENDING", "targetUrl": "https://ci.example.com/jobs/generate_simulator_debug_build", "detailsUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: run_unit_tests", "state": "PENDING", "targetUrl": "https://ci.example.com/jobs/run_unit_tests", "detailsUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: validate_release_build", "state": "PENDING", "targetUrl": "https://ci.example.com/jobs/validate_release_build", "detailsUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: preflight_check", "state": "SUCCESS", "targetUrl": "https://ci.example.com/jobs/preflight_check", "detailsUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "BLOCKED", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["ci/example: required_jobs_met", "version_health / assessment"], + "requiredStatusChecks": [{ "context": "ci/example: required_jobs_met" }, { "context": "version_health / assessment" }] + } + } + } + } + } + } + """ + private static let emptyRollupRequiredContextResult = """ { "data": { @@ -603,6 +797,88 @@ struct GitHubServiceBatchIntegrationTests { } """ + private static let otherPRGraphQLResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "number": 42, + "title": "Track required checks", + "url": "https://github.com/alice/repo/pull/42", + "author": { "login": "alice" }, + "updatedAt": "2024-01-01T00:00:00Z", + "labels": { + "nodes": [{ "id": "label-1", "name": "ci", "color": "0e8a16" }] + }, + "isDraft": false, + "state": "OPEN", + "headRefName": "feature/required-checks", + "statusCheckRollup": { + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "required_ci", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://ci.example.com/required", "context": null, "state": null, "targetUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": "APPROVED", + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["required_ci"], + "requiredStatusChecks": [{ "context": "required_ci" }] + } + } + } + } + } + } + """ + + private static let otherPRFailedRollupMissingRequiredContextResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "number": 42, + "title": "Track required checks", + "url": "https://github.com/alice/repo/pull/42", + "author": { "login": "alice" }, + "updatedAt": "2024-01-01T00:00:00Z", + "labels": { "nodes": [] }, + "isDraft": false, + "state": "OPEN", + "headRefName": "feature/required-checks", + "statusCheckRollup": { + "state": "FAILURE", + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "version_health / assessment", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://github.com/example/version-health", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: preflight_check", "state": "FAILURE", "targetUrl": "https://ci.example.com/preflight", "detailsUrl": null }, + { "__typename": "CheckRun", "name": "optional_cleanup", "status": "IN_PROGRESS", "conclusion": null, "isRequired": false, "detailsUrl": "https://ci.example.com/optional", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: optional_cleanup/approve", "state": "PENDING", "targetUrl": "https://ci.example.com/optional/approve", "detailsUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "BLOCKED", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["ci/example: required_jobs_met", "version_health / assessment"], + "requiredStatusChecks": [{ "context": "ci/example: required_jobs_met" }, { "context": "version_health / assessment" }] + } + } + } + } + } + } + """ + @Test func fetchAllOpenPRsUsesBatchGraphQLInsteadOfPerPRView() async throws { let mock = MockShellExecutor( executeResponseMatchers: [ @@ -680,7 +956,7 @@ struct GitHubServiceBatchIntegrationTests { #expect(result.pullRequests.first?.buildStatus == .pending) } - @Test func fetchAllOpenPRsTreatsMissingRequiredContextAsPendingAndSummarizesApprovalGate() async throws { + @Test func fetchAllOpenPRsTreatsFailedRollupWithMissingRequiredContextAsFailure() async throws { let mock = MockShellExecutor( executeResponseMatchers: [ ("--author=@me", .success(Self.authoredSearchResult)), @@ -692,11 +968,80 @@ struct GitHubServiceBatchIntegrationTests { let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) let pr = try #require(result.pullRequests.first) + #expect(pr.buildStatus == .failure) + #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 waiting for approval"]) + let blockingFailures = pr.statusChecks.filter { + !$0.isNonBlocking && ($0.status == .failure || $0.status == .error) + }.map(\.name) + #expect(blockingFailures == ["ci/circleci: preflight_check", "pull_requests"]) + } + + @Test func fetchAllOpenPRsTreatsMissingRequiredContextWithoutStartedCIAsNotStarted() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.missingRequiredContextNotStartedResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + let pr = try #require(result.pullRequests.first) + + #expect(pr.buildStatus == .notStarted) + #expect(pr.nonBlockingCheckSummary == nil) + } + + @Test func fetchAllOpenPRsKeepsRequiredWorkflowProgressOutOfNonBlockingSummary() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.missingRequiredContextWithRequiredWorkflowProgressResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + let pr = try #require(result.pullRequests.first) + #expect(pr.buildStatus == .pending) #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 waiting for approval"]) + #expect(pr.statusChecks.filter(\.isNonBlocking).map(\.name) == ["ci/example: dead_code_cleanup/approve_dead_code_cleanup"]) + } + + @Test func fetchAllOpenPRsTreatsApprovalNamedStatusContextWithoutGateAsPending() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.optionalApprovalNamedStatusContextResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + let pr = try #require(result.pullRequests.first) + + #expect(pr.buildStatus == .success) + #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 pending"]) + } + + @Test func fetchAllOpenPRsDoesNotLetRequiredApprovalGateSuppressOptionalParentCheck() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.requiredApprovalGateDoesNotSuppressOptionalParentResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + let pr = try #require(result.pullRequests.first) + + #expect(pr.buildStatus == .pending) + #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 running"]) } - @Test func fetchAllOpenPRsTreatsEmptyRollupWithRequiredContextAsPending() async throws { + @Test func fetchAllOpenPRsTreatsEmptyRollupWithRequiredContextAsNotStarted() async throws { let mock = MockShellExecutor( executeResponseMatchers: [ ("--author=@me", .success(Self.authoredSearchResult)), @@ -707,6 +1052,42 @@ struct GitHubServiceBatchIntegrationTests { let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) - #expect(result.pullRequests.first?.buildStatus == .pending) + #expect(result.pullRequests.first?.buildStatus == .notStarted) + } + + @Test func fetchOtherPRUsesSingleGraphQLRequest() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("graphql", .success(Self.otherPRGraphQLResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + let id = OtherPRIdentifier(host: "github.com", owner: "alice", repo: "repo", number: 42) + + let pr = try #require(await service.fetchOtherPR(id, enableInactiveDetection: false, inactiveThresholdDays: 3)) + let calls = await mock.executeCalls + + #expect(pr.number == 42) + #expect(pr.title == "Track required checks") + #expect(pr.headRefName == "feature/required-checks") + #expect(pr.buildStatus == .success) + #expect(pr.statusChecks.map(\.name) == ["required_ci"]) + #expect(calls.filter { $0.arguments.contains("graphql") }.count == 1) + #expect(calls.filter { $0.arguments.contains("pr") && $0.arguments.contains("view") }.isEmpty) + } + + @Test func fetchOtherPRTreatsFailedRollupWithMissingRequiredContextAsFailure() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("graphql", .success(Self.otherPRFailedRollupMissingRequiredContextResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + let id = OtherPRIdentifier(host: "github.com", owner: "alice", repo: "repo", number: 42) + + let pr = try #require(await service.fetchOtherPR(id, enableInactiveDetection: false, inactiveThresholdDays: 3)) + + #expect(pr.buildStatus == .failure) + #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 waiting for approval"]) } } diff --git a/MonitorLizardTests/PRMonitorViewModelTests.swift b/MonitorLizardTests/PRMonitorViewModelTests.swift index aa85374..c36a6c8 100644 --- a/MonitorLizardTests/PRMonitorViewModelTests.swift +++ b/MonitorLizardTests/PRMonitorViewModelTests.swift @@ -163,6 +163,7 @@ struct ResolveReviewDecisionTests { } } +@MainActor struct ParsePRURLTests { @Test func validGitHubURL() { @@ -213,6 +214,7 @@ struct ParsePRURLTests { } } +@MainActor struct OtherPRsServiceTests { private func makeService() -> OtherPRsService { @@ -277,6 +279,7 @@ struct OtherPRsServiceTests { } } +@MainActor struct PRTypeDisplayTitleTests { @Test(arguments: [0, 1, 5]) @@ -295,6 +298,7 @@ struct PRTypeDisplayTitleTests { } } +@MainActor struct CustomNamesServiceTests { private func makeService() -> CustomNamesService { From d013d098ffe9d130df515351ac77c1d41a699b0e Mon Sep 17 00:00:00 2001 From: Luke LaBonte Date: Mon, 11 May 2026 14:19:26 -0500 Subject: [PATCH 3/4] Handle rollup-only CI status fallback Use statusCheckRollup.state when required metadata and individual checks are unavailable, including EXPECTED as pending. Expand required-CI and parameterized test coverage around the fallback paths. --- MonitorLizard/Services/GitHubService.swift | 16 + MonitorLizardTests/GitHubServiceTests.swift | 562 ++++++++++++++---- .../PRMonitorViewModelTests.swift | 322 +++++----- MonitorLizardTests/UpdateServiceTests.swift | 24 +- 4 files changed, 608 insertions(+), 316 deletions(-) diff --git a/MonitorLizard/Services/GitHubService.swift b/MonitorLizard/Services/GitHubService.swift index d80558c..8277038 100644 --- a/MonitorLizard/Services/GitHubService.swift +++ b/MonitorLizard/Services/GitHubService.swift @@ -572,6 +572,7 @@ class GitHubService: ObservableObject { requiredStatusCheckContexts: requiredStatusCheckContexts ) let checksToEvaluate = statusChecksForOverallStatus(checks, requiredStatusCheckContexts: requiredStatusCheckContexts) + let hasRequiredMetadata = requiredStatusCheckContexts != nil || checks.contains { $0.isRequired != nil } // Priority: conflict > failure > error > pending > success var hasFailure = false @@ -579,6 +580,21 @@ class GitHubService: ObservableObject { var hasPending = !missingRequiredContexts.isEmpty var hasSuccess = false + if !hasRequiredMetadata && checksToEvaluate.isEmpty { + switch statusCheckRollupState?.uppercased() { + case "FAILURE": + hasFailure = true + case "ERROR": + hasError = true + case "PENDING", "EXPECTED": + hasPending = true + case "SUCCESS": + hasSuccess = true + default: + break + } + } + if !missingRequiredContexts.isEmpty { switch statusCheckRollupState?.uppercased() { case "FAILURE": diff --git a/MonitorLizardTests/GitHubServiceTests.swift b/MonitorLizardTests/GitHubServiceTests.swift index cb87615..ddf73ff 100644 --- a/MonitorLizardTests/GitHubServiceTests.swift +++ b/MonitorLizardTests/GitHubServiceTests.swift @@ -4,27 +4,29 @@ import Foundation // MARK: - Build Status Presentation Tests -@MainActor struct BuildStatusPresentationTests { + private static let presentations: [(status: BuildStatus, displayName: String, icon: String, systemImageName: String?)] = [ + (.conflict, "Merge Conflict", "❗", nil), + (.notStarted, "Not started", "🛑", "play.slash"), + (.pending, "Pending", "🔄", "gear"), + (.success, "Success", "✅", "gear.badge.checkmark"), + (.failure, "Failed", "❌", "gear.badge.xmark"), + (.error, "Error", "⚠️", "gear.badge.xmark"), + (.unknown, "Unknown", "❓", nil), + (.inactive, "Inactive", "⏳", nil), + ] + @Test func buildStatusPresentationCoversEveryState() { - let presentations: [(status: BuildStatus, displayName: String, icon: String, systemImageName: String?)] = [ - (.conflict, "Merge Conflict", "❗", nil), - (.notStarted, "Not started", "🛑", "play.slash"), - (.pending, "Pending", "🔄", "gear"), - (.success, "Success", "✅", "gear.badge.checkmark"), - (.failure, "Failed", "❌", "gear.badge.xmark"), - (.error, "Error", "⚠️", "gear.badge.xmark"), - (.unknown, "Unknown", "❓", nil), - (.inactive, "Inactive", "⏳", nil), - ] + #expect(Self.presentations.map(\.status) == BuildStatus.allCases) + } - #expect(presentations.map(\.status) == BuildStatus.allCases) - for presentation in presentations { - #expect(presentation.status.displayName == presentation.displayName) - #expect(presentation.status.icon == presentation.icon) - #expect(presentation.status.systemImageName == presentation.systemImageName) - } + @Test(arguments: BuildStatusPresentationTests.presentations) + @MainActor + func buildStatusPresentationMatchesExpected(status: BuildStatus, displayName: String, icon: String, systemImageName: String?) { + #expect(status.displayName == displayName) + #expect(status.icon == icon) + #expect(status.systemImageName == systemImageName) } } @@ -33,6 +35,81 @@ struct BuildStatusPresentationTests { @MainActor struct NonBlockingCheckSummaryTests { + enum NilSummaryScenario: CaseIterable, Sendable { + case requiredOnly + case unknownRequiredness + case allNonBlockingChecksPassed + case nonRequiredCheckNotMarkedNonBlocking + + @MainActor + var statusChecks: [StatusCheck] { + switch self { + case .requiredOnly: + return [Self.check(id: "required", status: .success, isRequired: true, isNonBlocking: false)] + case .unknownRequiredness: + return [Self.check(id: "unknown", status: .pending, isRequired: nil, isNonBlocking: false)] + case .allNonBlockingChecksPassed: + return [Self.check(id: "optional", status: .success)] + case .nonRequiredCheckNotMarkedNonBlocking: + return [Self.check(id: "required-workflow-job", status: .running, isNonBlocking: false)] + } + } + + @MainActor + private static func check(id: String, status: CheckStatus, isRequired: Bool? = false, isNonBlocking: Bool = true) -> StatusCheck { + StatusCheck(id: id, name: id, status: status, detailsUrl: nil, isRequired: isRequired, isNonBlocking: isNonBlocking) + } + } + + enum SegmentSummaryScenario: CaseIterable, Sendable { + case waitingAndRunning + case allActionableStates + case failedWithPassedCheck + + @MainActor + var statusChecks: [StatusCheck] { + switch self { + case .waitingAndRunning: + return [ + Self.check(id: "approval", status: .waiting), + Self.check(id: "analysis", status: .running), + ] + case .allActionableStates: + return [ + Self.check(id: "optional-failed", status: .failure), + Self.check(id: "optional-error", status: .error), + Self.check(id: "optional-waiting", status: .waiting), + Self.check(id: "optional-running", status: .running), + Self.check(id: "optional-queued", status: .queued), + Self.check(id: "optional-pending", status: .pending), + Self.check(id: "optional-success", status: .success), + Self.check(id: "optional-skipped", status: .skipped), + ] + case .failedWithPassedCheck: + return [ + Self.check(id: "optional-failed", status: .failure), + Self.check(id: "optional-passed", status: .success), + ] + } + } + + var expectedSegments: [String] { + switch self { + case .waitingAndRunning: + return ["1 waiting for approval", "1 running"] + case .allActionableStates: + return ["2 failed", "1 waiting for approval", "1 running", "1 queued", "1 pending"] + case .failedWithPassedCheck: + return ["1 failed"] + } + } + + @MainActor + private static func check(id: String, status: CheckStatus) -> StatusCheck { + StatusCheck(id: id, name: id, status: status, detailsUrl: nil, isRequired: false, isNonBlocking: true) + } + } + private func makePR(statusChecks: [StatusCheck]) -> PullRequest { PullRequest( number: 1, @@ -53,75 +130,20 @@ struct NonBlockingCheckSummaryTests { ) } - @Test func summaryIsNilWhenOnlyRequiredChecksExist() { - let pr = makePR(statusChecks: [ - StatusCheck(id: "required", name: "required", status: .success, detailsUrl: nil, isRequired: true) - ]) - - #expect(pr.nonBlockingCheckSummary == nil) - } - - @Test func summaryIsNilWhenRequirednessIsUnknown() { - let pr = makePR(statusChecks: [ - StatusCheck(id: "unknown", name: "unknown", status: .pending, detailsUrl: nil, isRequired: nil) - ]) - - #expect(pr.nonBlockingCheckSummary == nil) - } - - @Test func summaryIsNilWhenAllNonBlockingChecksPassed() { - let pr = makePR(statusChecks: [ - StatusCheck(id: "optional", name: "optional", status: .success, detailsUrl: nil, isRequired: false, isNonBlocking: true) - ]) + @Test(arguments: NilSummaryScenario.allCases) + func summaryIsNil(scenario: NilSummaryScenario) { + let pr = makePR(statusChecks: scenario.statusChecks) #expect(pr.nonBlockingCheckSummary == nil) } - @Test func summaryDescribesWaitingAndRunningNonBlockingChecks() throws { - let pr = makePR(statusChecks: [ - StatusCheck(id: "approval", name: "approval", status: .waiting, detailsUrl: nil, isRequired: false, isNonBlocking: true), - StatusCheck(id: "analysis", name: "analysis", status: .running, detailsUrl: nil, isRequired: false, isNonBlocking: true) - ]) - - let summary = try #require(pr.nonBlockingCheckSummary) - - #expect(summary.segments.map(\.text) == ["1 waiting for approval", "1 running"]) - } - - @Test func summaryOrdersAllActionableStatesAndOmitsPassedStates() throws { - let pr = makePR(statusChecks: [ - StatusCheck(id: "optional-failed", name: "optional-failed", status: .failure, detailsUrl: nil, isRequired: false, isNonBlocking: true), - StatusCheck(id: "optional-error", name: "optional-error", status: .error, detailsUrl: nil, isRequired: false, isNonBlocking: true), - StatusCheck(id: "optional-waiting", name: "optional-waiting", status: .waiting, detailsUrl: nil, isRequired: false, isNonBlocking: true), - StatusCheck(id: "optional-running", name: "optional-running", status: .running, detailsUrl: nil, isRequired: false, isNonBlocking: true), - StatusCheck(id: "optional-queued", name: "optional-queued", status: .queued, detailsUrl: nil, isRequired: false, isNonBlocking: true), - StatusCheck(id: "optional-pending", name: "optional-pending", status: .pending, detailsUrl: nil, isRequired: false, isNonBlocking: true), - StatusCheck(id: "optional-success", name: "optional-success", status: .success, detailsUrl: nil, isRequired: false, isNonBlocking: true), - StatusCheck(id: "optional-skipped", name: "optional-skipped", status: .skipped, detailsUrl: nil, isRequired: false, isNonBlocking: true), - ]) - - let summary = try #require(pr.nonBlockingCheckSummary) - - #expect(summary.segments.map(\.text) == ["2 failed", "1 waiting for approval", "1 running", "1 queued", "1 pending"]) - } - - @Test func summaryOmitsPassedNonBlockingChecksWhenAttentionIsNeeded() throws { - let pr = makePR(statusChecks: [ - StatusCheck(id: "optional-failed", name: "optional-failed", status: .failure, detailsUrl: nil, isRequired: false, isNonBlocking: true), - StatusCheck(id: "optional-passed", name: "optional-passed", status: .success, detailsUrl: nil, isRequired: false, isNonBlocking: true) - ]) + @Test(arguments: SegmentSummaryScenario.allCases) + func summarySegmentsMatchExpected(scenario: SegmentSummaryScenario) throws { + let pr = makePR(statusChecks: scenario.statusChecks) let summary = try #require(pr.nonBlockingCheckSummary) - #expect(summary.segments.map(\.text) == ["1 failed"]) - } - - @Test func summaryIgnoresNonRequiredChecksThatAreNotNonBlocking() { - let pr = makePR(statusChecks: [ - StatusCheck(id: "required-workflow-job", name: "required-workflow-job", status: .running, detailsUrl: nil, isRequired: false) - ]) - - #expect(pr.nonBlockingCheckSummary == nil) + #expect(summary.segments.map(\.text) == scenario.expectedSegments) } } @@ -198,6 +220,29 @@ struct GitHubServiceHostCacheTests { @MainActor struct GitHubServiceFetchErrorTests { + enum GitHubErrorMappingScenario: CaseIterable, Sendable { + case networkError + case commandNotFound + + var shellError: ShellError { + switch self { + case .networkError: + return .networkError("error connecting to api.github.com") + case .commandNotFound: + return .commandNotFound + } + } + + var expectedError: GitHubError { + switch self { + case .networkError: + return .networkError + case .commandNotFound: + return .notInstalled + } + } + } + /// When all fetches fail with a generic execution error (e.g. auth expired), the original /// ShellError should propagate — not be swallowed into GitHubError.networkError. @Test func executionFailureRethrowsAsShellError() async { @@ -216,31 +261,16 @@ struct GitHubServiceFetchErrorTests { } } - /// A genuine network error from the shell (gh CLI) should map to GitHubError.networkError. - @Test func shellNetworkErrorBecomesGitHubNetworkError() async { - let mock = MockShellExecutor(executeResponse: .failure(ShellError.networkError("error connecting to api.github.com"))) + @Test(arguments: GitHubErrorMappingScenario.allCases) + func shellFailureMapsToExpectedGitHubError(scenario: GitHubErrorMappingScenario) async { + let mock = MockShellExecutor(executeResponse: .failure(scenario.shellError)) let service = GitHubService(shellExecutor: mock) do { _ = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) Issue.record("Expected an error to be thrown") } catch let error as GitHubError { - #expect(error == .networkError) - } catch { - Issue.record("Unexpected error type: \(error)") - } - } - - /// A missing gh binary should map to GitHubError.notInstalled. - @Test func commandNotFoundBecomesGitHubNotInstalled() async { - let mock = MockShellExecutor(executeResponse: .failure(ShellError.commandNotFound)) - let service = GitHubService(shellExecutor: mock) - - do { - _ = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) - Issue.record("Expected an error to be thrown") - } catch let error as GitHubError { - #expect(error == .notInstalled) + #expect(error == scenario.expectedError) } catch { Issue.record("Unexpected error type: \(error)") } @@ -251,6 +281,21 @@ struct GitHubServiceFetchErrorTests { struct GitHubServiceBatchQueryTests { + enum RequiredMetadataQueryScenario: CaseIterable, Sendable { + case batch + case detail + + var query: String { + let request = PRStatusRequest(owner: "alice", repo: "repo", number: 42) + switch self { + case .batch: + return GitHubService.buildBatchQuery(for: [request]) + case .detail: + return GitHubService.buildPRDetailQuery(for: request) + } + } + } + @Test func buildBatchQueryContainsAllPRs() { let requests = [ PRStatusRequest(owner: "alice", repo: "widgets", number: 42), @@ -281,22 +326,9 @@ struct GitHubServiceBatchQueryTests { #expect(query.contains("reviewRequests")) } - @Test func buildBatchQueryIncludesRequiredCheckMetadata() { - let query = GitHubService.buildBatchQuery(for: [ - PRStatusRequest(owner: "alice", repo: "repo", number: 42) - ]) - - #expect(query.contains("isRequired(pullRequestNumber: 42)")) - #expect(query.components(separatedBy: "isRequired(pullRequestNumber: 42)").count - 1 == 2) - #expect(query.contains("baseRef")) - #expect(query.contains("branchProtectionRule")) - #expect(query.contains("requiredStatusCheckContexts")) - #expect(query.contains("requiredStatusChecks")) - #expect(query.range(of: #"statusCheckRollup\s*\{\s*state"#, options: .regularExpression) != nil) - } - - @Test func buildPRDetailQueryIncludesRequiredCheckMetadata() { - let query = GitHubService.buildPRDetailQuery(for: PRStatusRequest(owner: "alice", repo: "repo", number: 42)) + @Test(arguments: RequiredMetadataQueryScenario.allCases) + func queryIncludesRequiredCheckMetadata(scenario: RequiredMetadataQueryScenario) { + let query = scenario.query #expect(query.contains("isRequired(pullRequestNumber: 42)")) #expect(query.components(separatedBy: "isRequired(pullRequestNumber: 42)").count - 1 == 2) @@ -474,6 +506,65 @@ struct GitHubServiceBatchResponseParsingTests { let result = try GitHubService.parseBatchResponse(json, requests: [request]) #expect(result[request]?.reviewRequests?.first?.login == nil) } + + @Test func parseBatchResponseUnionsAndDeduplicatesRequiredStatusContexts() throws { + let json = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "main", + "statusCheckRollup": null, + "mergeable": null, + "mergeStateStatus": null, + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["legacy_ci", "duplicate_ci"], + "requiredStatusChecks": [{ "context": "modern_ci" }, { "context": "duplicate_ci" }] + } + } + } + } + } + } + """ + let request = PRStatusRequest(owner: "owner", repo: "repo", number: 1) + + let result = try GitHubService.parseBatchResponse(json, requests: [request]) + let contexts = try #require(result[request]?.requiredStatusCheckContexts) + + #expect(Set(contexts) == ["legacy_ci", "modern_ci", "duplicate_ci"]) + #expect(contexts.count == 3) + } + + @Test func parseBatchResponseLeavesRequiredContextsNilWithoutBranchProtectionRule() throws { + let json = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "main", + "statusCheckRollup": null, + "mergeable": null, + "mergeStateStatus": null, + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { "branchProtectionRule": null } + } + } + } + } + """ + let request = PRStatusRequest(owner: "owner", repo: "repo", number: 1) + + let result = try GitHubService.parseBatchResponse(json, requests: [request]) + + #expect(result[request]?.requiredStatusCheckContexts == nil) + } } // MARK: - Batch Integration Tests @@ -513,6 +604,30 @@ struct GitHubServiceBatchIntegrationTests { } """ + private static func rollupStateOnlyResult(state: String) -> String { + """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "state": "\(state)", + "contexts": { "nodes": [] } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { "branchProtectionRule": null } + } + } + } + } + """ + } + private static let requiredSuccessOptionalPendingResult = """ { "data": { @@ -606,6 +721,68 @@ struct GitHubServiceBatchIntegrationTests { } """ + private static let branchProtectionRequirednessFallbackResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "required_ci", "status": "COMPLETED", "conclusion": "SUCCESS", "detailsUrl": "https://ci.example.com/required", "context": null, "state": null, "targetUrl": null }, + { "__typename": "CheckRun", "name": "optional_ci", "status": "PENDING", "conclusion": null, "detailsUrl": "https://ci.example.com/optional", "context": null, "state": null, "targetUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["required_ci"], + "requiredStatusChecks": [{ "context": "required_ci" }] + } + } + } + } + } + } + """ + + private static let approvalContainsGateResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "required_ci", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://ci.example.com/required", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: deploy/wait_for_approval", "state": "PENDING", "targetUrl": "https://ci.example.com/deploy/wait", "detailsUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["required_ci"], + "requiredStatusChecks": [{ "context": "required_ci" }] + } + } + } + } + } + } + """ + private static let requiredApprovalGateDoesNotSuppressOptionalParentResult = """ { "data": { @@ -700,6 +877,38 @@ struct GitHubServiceBatchIntegrationTests { } """ + private static let requiredContextMissingErrorResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "state": "ERROR", + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "version_health / assessment", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://github.com/example/version-health", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: preflight_check", "state": "ERROR", "targetUrl": "https://ci.example.com/preflight", "detailsUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "BLOCKED", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["ci/example: required_jobs_met", "version_health / assessment"], + "requiredStatusChecks": [{ "context": "ci/example: required_jobs_met" }, { "context": "version_health / assessment" }] + } + } + } + } + } + } + """ + private static let missingRequiredContextNotStartedResult = """ { "data": { @@ -914,6 +1123,27 @@ struct GitHubServiceBatchIntegrationTests { #expect(result.pullRequests.first?.headRefName == "feature/test") } + @Test(arguments: [ + ("FAILURE", BuildStatus.failure), + ("ERROR", .error), + ("PENDING", .pending), + ("EXPECTED", .pending), + ("SUCCESS", .success), + ] as [(String, BuildStatus)]) + func fetchAllOpenPRsUsesRollupStateWhenNoCheckMetadata(state: String, expectedStatus: BuildStatus) async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.rollupStateOnlyResult(state: state))) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + + #expect(result.pullRequests.first?.buildStatus == expectedStatus) + } + @Test func fetchAllOpenPRsTreatsOptionalPendingChecksAsSuccessWhenRequiredChecksPass() async throws { let mock = MockShellExecutor( executeResponseMatchers: [ @@ -942,6 +1172,28 @@ struct GitHubServiceBatchIntegrationTests { #expect(result.pullRequests.first?.buildStatus == .success) } + @Test func fetchAllOpenPRsFallsBackToBranchProtectionWhenIsRequiredIsMissing() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.branchProtectionRequirednessFallbackResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + let pr = try #require(result.pullRequests.first) + let requiredCheck = try #require(pr.statusChecks.first { $0.name == "required_ci" }) + let optionalCheck = try #require(pr.statusChecks.first { $0.name == "optional_ci" }) + + #expect(pr.buildStatus == .success) + #expect(requiredCheck.isRequired == true) + #expect(requiredCheck.isNonBlocking == false) + #expect(optionalCheck.isRequired == false) + #expect(optionalCheck.isNonBlocking == true) + #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 pending"]) + } + @Test func fetchAllOpenPRsKeepsPendingStatusWhenRequiredMetadataIsUnknown() async throws { let mock = MockShellExecutor( executeResponseMatchers: [ @@ -976,6 +1228,26 @@ struct GitHubServiceBatchIntegrationTests { #expect(blockingFailures == ["ci/circleci: preflight_check", "pull_requests"]) } + @Test func fetchAllOpenPRsTreatsErrorRollupWithMissingRequiredContextAsError() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.requiredContextMissingErrorResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + let pr = try #require(result.pullRequests.first) + let blockingFailures = pr.statusChecks.filter { + !$0.isNonBlocking && ($0.status == .failure || $0.status == .error) + }.map(\.name) + + #expect(pr.buildStatus == .error) + #expect(blockingFailures == ["ci/example: preflight_check"]) + #expect(pr.nonBlockingCheckSummary == nil) + } + @Test func fetchAllOpenPRsTreatsMissingRequiredContextWithoutStartedCIAsNotStarted() async throws { let mock = MockShellExecutor( executeResponseMatchers: [ @@ -1025,6 +1297,25 @@ struct GitHubServiceBatchIntegrationTests { #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 pending"]) } + @Test func fetchAllOpenPRsTreatsApprovalNamedGateAsWaiting() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.approvalContainsGateResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + let pr = try #require(result.pullRequests.first) + let approvalCheck = try #require(pr.statusChecks.first { $0.name == "ci/example: deploy/wait_for_approval" }) + + #expect(pr.buildStatus == .success) + #expect(approvalCheck.status == .waiting) + #expect(approvalCheck.isNonBlocking == true) + #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 waiting for approval"]) + } + @Test func fetchAllOpenPRsDoesNotLetRequiredApprovalGateSuppressOptionalParentCheck() async throws { let mock = MockShellExecutor( executeResponseMatchers: [ @@ -1055,6 +1346,49 @@ struct GitHubServiceBatchIntegrationTests { #expect(result.pullRequests.first?.buildStatus == .notStarted) } + @Test func fetchPRStatusUsesRequiredCIStatusParsing() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("graphql", .success(Self.requiredContextMissingApprovalWaitingResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let status = try await service.fetchPRStatus( + owner: "alice", + repo: "repo", + number: 1, + updatedAt: Date(), + enableInactiveDetection: false, + inactiveThresholdDays: 3 + ) + + #expect(status.status == .failure) + #expect(status.headRefName == "feature/test") + #expect(status.statusChecks.filter(\.isNonBlocking).map(\.name) == ["ci/circleci: dead_code_cleanup/approve_dead_code_cleanup"]) + } + + @Test func fetchPRStatusTreatsMissingRequiredContextWithoutStartedCIAsNotStarted() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("graphql", .success(Self.missingRequiredContextNotStartedResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let status = try await service.fetchPRStatus( + owner: "alice", + repo: "repo", + number: 1, + updatedAt: Date(), + enableInactiveDetection: false, + inactiveThresholdDays: 3 + ) + + #expect(status.status == .notStarted) + #expect(status.statusChecks.allSatisfy { !$0.isNonBlocking }) + } + @Test func fetchOtherPRUsesSingleGraphQLRequest() async throws { let mock = MockShellExecutor( executeResponseMatchers: [ diff --git a/MonitorLizardTests/PRMonitorViewModelTests.swift b/MonitorLizardTests/PRMonitorViewModelTests.swift index c36a6c8..5daa8a5 100644 --- a/MonitorLizardTests/PRMonitorViewModelTests.swift +++ b/MonitorLizardTests/PRMonitorViewModelTests.swift @@ -4,212 +4,158 @@ import Foundation struct ResolveReviewDecisionTests { - private typealias Review = GHPRDetailResponse.Review - private typealias ReviewAuthor = GHPRDetailResponse.Review.ReviewAuthor - private typealias ReviewRequest = GHPRDetailResponse.ReviewRequest - - private func review(_ login: String, state: String) -> Review { - Review(author: ReviewAuthor(login: login), state: state) - } - - private func request(_ login: String) -> ReviewRequest { - ReviewRequest(login: login) - } - - // MARK: Non-CHANGES_REQUESTED pass-throughs - - @Test func approvedPassesThrough() { - let result = GitHubService.resolveReviewDecision(rawValue: "APPROVED", latestReviews: nil, reviewRequests: nil) - #expect(result == .approved) - } - - @Test func reviewRequiredPassesThrough() { - let result = GitHubService.resolveReviewDecision(rawValue: "REVIEW_REQUIRED", latestReviews: nil, reviewRequests: nil) - #expect(result == .reviewRequired) - } - - @Test func nilRawValueReturnsNil() { - let result = GitHubService.resolveReviewDecision(rawValue: nil, latestReviews: nil, reviewRequests: nil) - #expect(result == nil) - } - - // MARK: CHANGES_REQUESTED with no re-request - - @Test func changesRequestedWithNoReviewRequestsStaysChangesRequested() { - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: [review("alice", state: "CHANGES_REQUESTED")], - reviewRequests: [] - ) - #expect(result == .changesRequested) - } - - @Test func changesRequestedWithNilReviewRequestsStaysChangesRequested() { - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: [review("alice", state: "CHANGES_REQUESTED")], - reviewRequests: nil - ) - #expect(result == .changesRequested) - } - - @Test func changesRequestedWhenDifferentReviewerReRequestedStaysChangesRequested() { - // alice requested changes, but only bob has a pending re-review - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: [review("alice", state: "CHANGES_REQUESTED")], - reviewRequests: [request("bob")] - ) - #expect(result == .changesRequested) - } - - @Test func changesRequestedWhenOnlyOneOfTwoReRequestedStaysChangesRequested() { - // Both alice and bob requested changes; only alice re-requested - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: [ - review("alice", state: "CHANGES_REQUESTED"), - review("bob", state: "CHANGES_REQUESTED") - ], - reviewRequests: [request("alice")] - ) - #expect(result == .changesRequested) - } - - // MARK: CHANGES_REQUESTED downgraded to REVIEW_REQUIRED - - @Test func changesRequestedDowngradedWhenReviewerReRequested() { - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: [review("alice", state: "CHANGES_REQUESTED")], - reviewRequests: [request("alice")] - ) - #expect(result == .reviewRequired) - } - - @Test func changesRequestedDowngradedWhenAllReviewersReRequested() { - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: [ - review("alice", state: "CHANGES_REQUESTED"), - review("bob", state: "CHANGES_REQUESTED") - ], - reviewRequests: [request("alice"), request("bob")] - ) - #expect(result == .reviewRequired) - } - - @Test func changesRequestedDowngradedWhenMixedReviewsAndAllChangesReRequesters() { - // alice approved, bob requested changes and has been re-requested - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: [ - review("alice", state: "APPROVED"), - review("bob", state: "CHANGES_REQUESTED") - ], - reviewRequests: [request("bob")] - ) - #expect(result == .reviewRequired) - } + typealias Review = GHPRDetailResponse.Review + typealias ReviewAuthor = GHPRDetailResponse.Review.ReviewAuthor + typealias ReviewRequest = GHPRDetailResponse.ReviewRequest + + enum Scenario: CaseIterable, Sendable { + case approvedPassesThrough + case reviewRequiredPassesThrough + case nilRawValueReturnsNil + case changesRequestedWithNoReviewRequests + case changesRequestedWithNilReviewRequests + case changesRequestedWhenDifferentReviewerReRequested + case changesRequestedWhenOnlyOneOfTwoReRequested + case changesRequestedDowngradedWhenReviewerReRequested + case changesRequestedDowngradedWhenAllReviewersReRequested + case changesRequestedDowngradedWhenMixedReviewsAndAllChangesReRequesters + case changesRequestedWithNoLatestReviewsAndPendingRequest + case changesRequestedWithEmptyLatestReviewsAndPendingRequest + case changesRequestedWithEmptyLatestReviewsAndNoRequests + case teamReviewRequestIgnoredGracefully + case caseInsensitiveRawValue + + var rawValue: String? { + switch self { + case .approvedPassesThrough: + return "APPROVED" + case .reviewRequiredPassesThrough: + return "REVIEW_REQUIRED" + case .nilRawValueReturnsNil: + return nil + case .caseInsensitiveRawValue: + return "changes_requested" + default: + return "CHANGES_REQUESTED" + } + } - // MARK: Edge cases + var latestReviews: [Review]? { + switch self { + case .approvedPassesThrough, .reviewRequiredPassesThrough, .nilRawValueReturnsNil, + .changesRequestedWithNoLatestReviewsAndPendingRequest: + return nil + case .changesRequestedWithEmptyLatestReviewsAndPendingRequest, + .changesRequestedWithEmptyLatestReviewsAndNoRequests: + return [] + case .changesRequestedWhenOnlyOneOfTwoReRequested, + .changesRequestedDowngradedWhenAllReviewersReRequested: + return [ + ResolveReviewDecisionTests.review("alice", state: "CHANGES_REQUESTED"), + ResolveReviewDecisionTests.review("bob", state: "CHANGES_REQUESTED"), + ] + case .changesRequestedDowngradedWhenMixedReviewsAndAllChangesReRequesters: + return [ + ResolveReviewDecisionTests.review("alice", state: "APPROVED"), + ResolveReviewDecisionTests.review("bob", state: "CHANGES_REQUESTED"), + ] + default: + return [ResolveReviewDecisionTests.review("alice", state: "CHANGES_REQUESTED")] + } + } - @Test func changesRequestedWithNoLatestReviewsAndPendingRequestBecomesReviewRequired() { - // GitHub sometimes returns latestReviews empty even when reviewDecision is CHANGES_REQUESTED. - // If there's a pending re-review, treat it as reviewRequired. - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: nil, - reviewRequests: [request("alice")] - ) - #expect(result == .reviewRequired) - } + var reviewRequests: [ReviewRequest]? { + switch self { + case .approvedPassesThrough, .reviewRequiredPassesThrough, .nilRawValueReturnsNil: + return nil + case .changesRequestedWithNoReviewRequests, .changesRequestedWithEmptyLatestReviewsAndNoRequests: + return [] + case .changesRequestedWithNilReviewRequests: + return nil + case .changesRequestedWhenDifferentReviewerReRequested: + return [ResolveReviewDecisionTests.request("bob")] + case .changesRequestedWhenOnlyOneOfTwoReRequested, + .changesRequestedDowngradedWhenReviewerReRequested, + .caseInsensitiveRawValue: + return [ResolveReviewDecisionTests.request("alice")] + case .changesRequestedDowngradedWhenAllReviewersReRequested: + return [ResolveReviewDecisionTests.request("alice"), ResolveReviewDecisionTests.request("bob")] + case .changesRequestedDowngradedWhenMixedReviewsAndAllChangesReRequesters: + return [ResolveReviewDecisionTests.request("bob")] + case .changesRequestedWithNoLatestReviewsAndPendingRequest, + .changesRequestedWithEmptyLatestReviewsAndPendingRequest: + return [ResolveReviewDecisionTests.request("alice")] + case .teamReviewRequestIgnoredGracefully: + return [ReviewRequest(login: nil)] + } + } - @Test func changesRequestedWithEmptyLatestReviewsAndPendingRequestBecomesReviewRequired() { - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: [], - reviewRequests: [request("alice")] - ) - #expect(result == .reviewRequired) + var expected: ReviewDecision? { + switch self { + case .approvedPassesThrough: + return .approved + case .reviewRequiredPassesThrough, + .changesRequestedDowngradedWhenReviewerReRequested, + .changesRequestedDowngradedWhenAllReviewersReRequested, + .changesRequestedDowngradedWhenMixedReviewsAndAllChangesReRequesters, + .changesRequestedWithNoLatestReviewsAndPendingRequest, + .changesRequestedWithEmptyLatestReviewsAndPendingRequest, + .caseInsensitiveRawValue: + return .reviewRequired + case .nilRawValueReturnsNil: + return nil + default: + return .changesRequested + } + } } - @Test func changesRequestedWithEmptyLatestReviewsAndNoRequestsStaysChangesRequested() { - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: [], - reviewRequests: [] - ) - #expect(result == .changesRequested) + private static func review(_ login: String, state: String) -> Review { + Review(author: ReviewAuthor(login: login), state: state) } - @Test func teamReviewRequestIgnoredGracefully() { - // Team review requests have nil login — should not crash or incorrectly downgrade - let result = GitHubService.resolveReviewDecision( - rawValue: "CHANGES_REQUESTED", - latestReviews: [review("alice", state: "CHANGES_REQUESTED")], - reviewRequests: [ReviewRequest(login: nil)] - ) - #expect(result == .changesRequested) + private static func request(_ login: String) -> ReviewRequest { + ReviewRequest(login: login) } - @Test func caseInsensitiveRawValue() { + @Test(arguments: Scenario.allCases) + func resolvesReviewDecision(scenario: Scenario) { let result = GitHubService.resolveReviewDecision( - rawValue: "changes_requested", - latestReviews: [review("alice", state: "CHANGES_REQUESTED")], - reviewRequests: [request("alice")] + rawValue: scenario.rawValue, + latestReviews: scenario.latestReviews, + reviewRequests: scenario.reviewRequests ) - #expect(result == .reviewRequired) + #expect(result == scenario.expected) } } @MainActor struct ParsePRURLTests { - @Test func validGitHubURL() { - let id = GitHubService.parsePRURL("https://github.com/owner/repo/pull/123") - #expect(id?.host == "github.com") - #expect(id?.owner == "owner") - #expect(id?.repo == "repo") - #expect(id?.number == 123) - } - - @Test func validGHEURL() { - let id = GitHubService.parsePRURL("https://github.example.com/myorg/myrepo/pull/42") - #expect(id?.host == "github.example.com") - #expect(id?.owner == "myorg") - #expect(id?.repo == "myrepo") - #expect(id?.number == 42) - } - - @Test func invalidURLNotPullPath() { - let id = GitHubService.parsePRURL("https://github.com/owner/repo/issues/123") - #expect(id == nil) - } - - @Test func invalidURLMissingNumber() { - let id = GitHubService.parsePRURL("https://github.com/owner/repo/pull/") - #expect(id == nil) - } - - @Test func invalidURLNonNumericNumber() { - let id = GitHubService.parsePRURL("https://github.com/owner/repo/pull/abc") - #expect(id == nil) - } - - @Test func invalidURLNoHost() { - let id = GitHubService.parsePRURL("not-a-url") - #expect(id == nil) - } - - @Test func invalidURLEmpty() { - let id = GitHubService.parsePRURL("") - #expect(id == nil) - } + @Test(arguments: [ + ("https://github.com/owner/repo/pull/123", "github.com", "owner", "repo", 123), + ("https://github.example.com/myorg/myrepo/pull/42", "github.example.com", "myorg", "myrepo", 42), + ] as [(String, String, String, String, Int)]) + func parsesValidPRURL(url: String, host: String, owner: String, repo: String, number: Int) { + let id = GitHubService.parsePRURL(url) + + #expect(id?.host == host) + #expect(id?.owner == owner) + #expect(id?.repo == repo) + #expect(id?.number == number) + } + + @Test(arguments: [ + "https://github.com/owner/repo/issues/123", + "https://github.com/owner/repo/pull/", + "https://github.com/owner/repo/pull/abc", + "not-a-url", + "", + "https://github.com/owner/repo/pull/123/files", + ]) + func rejectsInvalidPRURL(url: String) { + let id = GitHubService.parsePRURL(url) - @Test func validURLWithTrailingSlash() { - // Extra path components — should not match - let id = GitHubService.parsePRURL("https://github.com/owner/repo/pull/123/files") #expect(id == nil) } } diff --git a/MonitorLizardTests/UpdateServiceTests.swift b/MonitorLizardTests/UpdateServiceTests.swift index 6519150..5235c00 100644 --- a/MonitorLizardTests/UpdateServiceTests.swift +++ b/MonitorLizardTests/UpdateServiceTests.swift @@ -6,25 +6,21 @@ struct UpdateServiceTests { // MARK: isInformationalError - @Test func noUpdateAvailableErrorIsInformational() { + @Test(arguments: [ + ("SUSparkleErrorDomain", 1001), + ("SomeOtherDomain", 1001), + ] as [(String, Int)]) + func code1001IsInformational(domain: String, code: Int) { // Code 1001 = SUNoUpdateAvailableError. Sparkle already shows "You're up to date" // for this case, so we must not also show our own "Update Failed" alert. - let error = NSError(domain: "SUSparkleErrorDomain", code: 1001) + let error = NSError(domain: domain, code: code) #expect(UpdateService.isInformationalError(error)) } - @Test func otherSparkleErrorCodesAreNotInformational() { + @Test(arguments: [1000, 1002, 1003, 2001, -1]) + func otherSparkleErrorCodesAreNotInformational(code: Int) { // Any non-1001 code is a real failure and should surface an alert. - for code in [1000, 1002, 1003, 2001, -1] { - let error = NSError(domain: "SUSparkleErrorDomain", code: code) - #expect(!UpdateService.isInformationalError(error), "code \(code) should not be informational") - } - } - - @Test func errorDomainDoesNotAffectFiltering() { - // Filtering is keyed on code alone, not domain, since Sparkle may use - // multiple domains across versions. - let error = NSError(domain: "SomeOtherDomain", code: 1001) - #expect(UpdateService.isInformationalError(error)) + let error = NSError(domain: "SUSparkleErrorDomain", code: code) + #expect(!UpdateService.isInformationalError(error), "code \(code) should not be informational") } } From 8df841c5e349d7536ac64bd68042946be094b94c Mon Sep 17 00:00:00 2001 From: Luke LaBonte Date: Mon, 11 May 2026 14:57:49 -0500 Subject: [PATCH 4/4] Handle not-started CI edge cases Treat not-started required CI as needing attention and avoid counting waiting approval parent checks as active CI work. --- MonitorLizard/Services/GitHubService.swift | 50 +++++++--- MonitorLizard/Services/WatchlistService.swift | 4 +- .../ViewModels/PRMonitorViewModel.swift | 10 +- MonitorLizardTests/GitHubServiceTests.swift | 99 +++++++++++++++++++ .../PRMonitorViewModelTests.swift | 42 ++++++++ 5 files changed, 185 insertions(+), 20 deletions(-) diff --git a/MonitorLizard/Services/GitHubService.swift b/MonitorLizard/Services/GitHubService.swift index 8277038..9264e6d 100644 --- a/MonitorLizard/Services/GitHubService.swift +++ b/MonitorLizard/Services/GitHubService.swift @@ -657,7 +657,7 @@ class GitHubService: ObservableObject { return .error } - if !missingRequiredContexts.isEmpty && !hasActiveNonApprovalWork(in: checks) { + if !missingRequiredContexts.isEmpty && !hasActiveNonApprovalWork(in: checks, requiredStatusCheckContexts: requiredStatusCheckContexts) { return .notStarted } @@ -681,17 +681,32 @@ class GitHubService: ObservableObject { return .success } - private func hasActiveNonApprovalWork(in checks: [GHPRDetailResponse.StatusCheck]) -> Bool { - checks.contains { check in + private func hasActiveNonApprovalWork( + in checks: [GHPRDetailResponse.StatusCheck], + requiredStatusCheckContexts: [String]? + ) -> Bool { + let requiredContextSet = requiredStatusCheckContexts.map(Set.init) + let approvalParentWorkflowNames = approvalParentWorkflowNames(in: checks, requiredContextSet: requiredContextSet) + + return checks.contains { check in guard let checkName = check.name ?? check.context, - !isApprovalGate(checkName) else { + !isApprovalGate(checkName) else { return false } + let isRequired = check.isRequired ?? requiredContextSet.map { $0.contains(checkName) } + let isWaitingForApprovalParent = check.__typename == "CheckRun" + && isRequired == false + && approvalParentWorkflowNames.contains(checkName) + if let state = check.state?.uppercased(), ["PENDING", "EXPECTED"].contains(state) { return true } + if let status = check.status?.uppercased(), ["IN_PROGRESS", "WAITING"].contains(status), isWaitingForApprovalParent { + return false + } + if let status = check.status?.uppercased(), ["IN_PROGRESS", "QUEUED", "WAITING", "PENDING"].contains(status) { return true } @@ -723,6 +738,20 @@ class GitHubService: ObservableObject { return hasRequiredMetadata ? [] : checks } + private func approvalParentWorkflowNames( + in checks: [GHPRDetailResponse.StatusCheck], + requiredContextSet: Set? + ) -> Set { + Set(checks.compactMap { check -> String? in + guard let checkName = check.name ?? check.context, + (check.isRequired ?? requiredContextSet.map { $0.contains(checkName) }) == false, + ["PENDING", "EXPECTED"].contains(check.state?.uppercased()) else { + return nil + } + return parentWorkflowNameForApprovalGate(checkName) + }) + } + private func missingRequiredStatusCheckContexts( in checks: [GHPRDetailResponse.StatusCheck], requiredStatusCheckContexts: [String]? @@ -748,14 +777,7 @@ class GitHubService: ObservableObject { ) let rollupFailedWithMissingRequiredContext = !missingRequiredContexts.isEmpty && ["FAILURE", "ERROR"].contains(statusCheckRollupState?.uppercased()) - let pendingApprovalWorkflowNames = Set(checks.compactMap { check -> String? in - guard let checkName = check.name ?? check.context, - (check.isRequired ?? requiredContextSet.map { $0.contains(checkName) }) == false, - ["PENDING", "EXPECTED"].contains(check.state?.uppercased()) else { - return nil - } - return parentWorkflowNameForApprovalGate(checkName) - }) + let approvalParentWorkflowNames = approvalParentWorkflowNames(in: checks, requiredContextSet: requiredContextSet) return checks.compactMap { check in // Extract check name from either name (CheckRun) or context (StatusContext) @@ -821,8 +843,8 @@ class GitHubService: ObservableObject { && (checkStatus == .failure || checkStatus == .error) let isWaitingForApprovalParent = check.__typename == "CheckRun" && isRequired == false - && checkStatus == .running - && pendingApprovalWorkflowNames.contains(checkName) + && (checkStatus == .running || checkStatus == .waiting) + && approvalParentWorkflowNames.contains(checkName) let isRequiredAggregateWork = !missingRequiredContexts.isEmpty && !isApprovalGate(checkName) let isNonBlocking = isRequired == false && !isBlockingFailure diff --git a/MonitorLizard/Services/WatchlistService.swift b/MonitorLizard/Services/WatchlistService.swift index 8da39a1..14ac3d6 100644 --- a/MonitorLizard/Services/WatchlistService.swift +++ b/MonitorLizard/Services/WatchlistService.swift @@ -43,8 +43,8 @@ class WatchlistService { for pr in currentPRs { guard let watched = watchedPRs[pr.id] else { continue } - // Check if status changed from pending to any completed state - let wasIncomplete = watched.lastStatus == .pending || watched.lastStatus == .unknown + // Check if status changed from incomplete to any completed state + let wasIncomplete = watched.lastStatus == .notStarted || watched.lastStatus == .pending || watched.lastStatus == .unknown let isNowComplete = pr.buildStatus == .success || pr.buildStatus == .failure || pr.buildStatus == .error if wasIncomplete && isNowComplete { diff --git a/MonitorLizard/ViewModels/PRMonitorViewModel.swift b/MonitorLizard/ViewModels/PRMonitorViewModel.swift index 1527644..4a57403 100644 --- a/MonitorLizard/ViewModels/PRMonitorViewModel.swift +++ b/MonitorLizard/ViewModels/PRMonitorViewModel.swift @@ -58,7 +58,8 @@ class PRMonitorViewModel: ObservableObject { let allPRs = unsortedPullRequests + otherPullRequests return Set(allPRs.compactMap { pr -> String? in let badBuild = pr.buildStatus == .failure || pr.buildStatus == .error - || pr.buildStatus == .conflict || pr.buildStatus == .inactive + || pr.buildStatus == .conflict || pr.buildStatus == .notStarted + || pr.buildStatus == .inactive guard badBuild || pr.reviewDecision == .changesRequested else { return nil } return pr.repository.nameWithOwner }) @@ -298,11 +299,12 @@ class PRMonitorViewModel: ObservableObject { pullRequests = newPullRequests } - // Update warning icon indicator (failures, errors, conflicts, changes requested, inactive PRs, or any review PRs) + // Update warning icon indicator (failures, errors, conflicts, not-started/inactive PRs, changes requested, or any review PRs) let allDisplayed = newPullRequests + otherPullRequests let hasBadStatus = allDisplayed.contains { pr in let badBuild = pr.buildStatus == .failure || pr.buildStatus == .error - || pr.buildStatus == .conflict || pr.buildStatus == .inactive + || pr.buildStatus == .conflict || pr.buildStatus == .notStarted + || pr.buildStatus == .inactive return badBuild || pr.reviewDecision == .changesRequested } let hasReviewPRs = newPullRequests.contains { pr in @@ -383,7 +385,7 @@ class PRMonitorViewModel: ObservableObject { private func sort(_ prs: [PullRequest]) -> [PullRequest] { prs.sorted { pr1, pr2 in - let nonSuccessStatuses: [BuildStatus] = [.failure, .error, .conflict, .pending, .inactive] + let nonSuccessStatuses: [BuildStatus] = [.failure, .error, .conflict, .notStarted, .pending, .inactive] let pr1NonSuccess = nonSuccessStatuses.contains(pr1.buildStatus) || pr1.reviewDecision == .changesRequested let pr2NonSuccess = nonSuccessStatuses.contains(pr2.buildStatus) || pr2.reviewDecision == .changesRequested diff --git a/MonitorLizardTests/GitHubServiceTests.swift b/MonitorLizardTests/GitHubServiceTests.swift index ddf73ff..3c9d219 100644 --- a/MonitorLizardTests/GitHubServiceTests.swift +++ b/MonitorLizardTests/GitHubServiceTests.swift @@ -752,6 +752,38 @@ struct GitHubServiceBatchIntegrationTests { } """ + private static let requiredSuccessWaitingApprovalParentResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "required_ci", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://ci.example.com/required", "context": null, "state": null, "targetUrl": null }, + { "__typename": "CheckRun", "name": "deploy", "status": "WAITING", "conclusion": null, "isRequired": false, "detailsUrl": "https://ci.example.com/deploy", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: deploy/approve_deploy", "state": "PENDING", "targetUrl": "https://ci.example.com/deploy/approve", "detailsUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["required_ci"], + "requiredStatusChecks": [{ "context": "required_ci" }] + } + } + } + } + } + } + """ + private static let approvalContainsGateResult = """ { "data": { @@ -877,6 +909,39 @@ struct GitHubServiceBatchIntegrationTests { } """ + private static let missingRequiredContextWithWaitingApprovalParentResult = """ + { + "data": { + "pr0": { + "pullRequest": { + "headRefName": "feature/test", + "statusCheckRollup": { + "state": "PENDING", + "contexts": { + "nodes": [ + { "__typename": "CheckRun", "name": "version_health / assessment", "status": "COMPLETED", "conclusion": "SUCCESS", "isRequired": true, "detailsUrl": "https://github.com/example/version-health", "context": null, "state": null, "targetUrl": null }, + { "__typename": "CheckRun", "name": "deploy", "status": "WAITING", "conclusion": null, "isRequired": false, "detailsUrl": "https://ci.example.com/deploy", "context": null, "state": null, "targetUrl": null }, + { "__typename": "StatusContext", "name": null, "status": null, "conclusion": null, "isRequired": false, "context": "ci/example: deploy/approve_deploy", "state": "PENDING", "targetUrl": "https://ci.example.com/deploy/approve", "detailsUrl": null } + ] + } + }, + "mergeable": "MERGEABLE", + "mergeStateStatus": "BLOCKED", + "reviewDecision": null, + "latestReviews": { "nodes": [] }, + "reviewRequests": { "nodes": [] }, + "baseRef": { + "branchProtectionRule": { + "requiredStatusCheckContexts": ["ci/example: required_jobs_met", "version_health / assessment"], + "requiredStatusChecks": [{ "context": "ci/example: required_jobs_met" }, { "context": "version_health / assessment" }] + } + } + } + } + } + } + """ + private static let requiredContextMissingErrorResult = """ { "data": { @@ -1316,6 +1381,40 @@ struct GitHubServiceBatchIntegrationTests { #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 waiting for approval"]) } + @Test func fetchAllOpenPRsSuppressesWaitingApprovalParentFromNonBlockingSummary() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.requiredSuccessWaitingApprovalParentResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + let pr = try #require(result.pullRequests.first) + + #expect(pr.buildStatus == .success) + #expect(pr.statusChecks.filter(\.isNonBlocking).map(\.name) == ["ci/example: deploy/approve_deploy"]) + #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 waiting for approval"]) + } + + @Test func fetchAllOpenPRsIgnoresWaitingApprovalParentWhenRequiredCIHasNotStarted() async throws { + let mock = MockShellExecutor( + executeResponseMatchers: [ + ("--author=@me", .success(Self.authoredSearchResult)), + ("graphql", .success(Self.missingRequiredContextWithWaitingApprovalParentResult)) + ] + ) + let service = GitHubService(shellExecutor: mock) + + let result = try await service.fetchAllOpenPRs(enableInactiveDetection: false, inactiveThresholdDays: 3) + let pr = try #require(result.pullRequests.first) + + #expect(pr.buildStatus == .notStarted) + #expect(pr.statusChecks.filter(\.isNonBlocking).map(\.name) == ["ci/example: deploy/approve_deploy"]) + #expect(pr.nonBlockingCheckSummary?.segments.map(\.text) == ["1 waiting for approval"]) + } + @Test func fetchAllOpenPRsDoesNotLetRequiredApprovalGateSuppressOptionalParentCheck() async throws { let mock = MockShellExecutor( executeResponseMatchers: [ diff --git a/MonitorLizardTests/PRMonitorViewModelTests.swift b/MonitorLizardTests/PRMonitorViewModelTests.swift index 5daa8a5..9b86862 100644 --- a/MonitorLizardTests/PRMonitorViewModelTests.swift +++ b/MonitorLizardTests/PRMonitorViewModelTests.swift @@ -507,6 +507,27 @@ struct PRMonitorViewModelTests { return (WatchlistService(defaults: suite), OtherPRsService(defaults: suite)) } + private func makePR(number: Int = 1, nameWithOwner: String = "acme/widget", buildStatus: BuildStatus) -> PullRequest { + let name = String(nameWithOwner.split(separator: "/").last ?? "repo") + return PullRequest( + number: number, + title: "Test PR #\(number)", + repository: PullRequest.RepositoryInfo(name: name, nameWithOwner: nameWithOwner), + url: "https://github.com/\(nameWithOwner)/pull/\(number)", + author: PullRequest.Author(login: "testuser"), + headRefName: "feature/test", + updatedAt: Date(), + buildStatus: buildStatus, + isWatched: false, + labels: [], + type: .authored, + isDraft: false, + statusChecks: [], + reviewDecision: nil, + host: "github.com" + ) + } + private func createLoadedViewModel() async -> PRMonitorViewModel { let (watchlist, otherPRs) = makeIsolatedServices() let vm = PRMonitorViewModel(isDemoMode: true, watchlistService: watchlist, otherPRsService: otherPRs) @@ -533,6 +554,27 @@ struct PRMonitorViewModelTests { #expect(vm.selectedRepository == "All Repositories") } + @Test func reposWithIssuesIncludesNotStartedPRs() { + let (watchlist, otherPRs) = makeIsolatedServices() + let vm = PRMonitorViewModel(isDemoMode: true, watchlistService: watchlist, otherPRsService: otherPRs) + vm.stopPolling() + + vm.otherPullRequests = [makePR(buildStatus: .notStarted)] + + #expect(vm.reposWithIssues == ["acme/widget"]) + } + + @Test func watchlistReportsCompletionFromNotStarted() { + let (watchlist, _) = makeIsolatedServices() + let pendingPR = makePR(buildStatus: .notStarted) + var completedPR = pendingPR + completedPR.buildStatus = .success + + watchlist.watch(pendingPR) + + #expect(watchlist.checkForCompletions(currentPRs: [completedPR]).map(\.id) == [pendingPR.id]) + } + @Test func filterByRepository() async { let vm = await createLoadedViewModel() defer { UserDefaults.standard.removeObject(forKey: "selectedRepository") }