From 2aa65156a8f16bc36d546b6e5119a4ef847b90ac Mon Sep 17 00:00:00 2001 From: Olivier Halligon Date: Wed, 12 Feb 2020 16:32:13 +0100 Subject: [PATCH] [IOSP-569] Fix GitHub comments accidentally seen as PR reference (#54) * Fix #n in comment being parsed as PR reference Introducing a zero-width-space to avoid having GitHub detect this as a PR reference (even inside backticks) * Fix timezone used by JSONLogger Was making tests fail once I got back in the GMT+1 timezone * Improved textual description of queue To make it more readable when accessed directly in browser * swift test --generate-linuxmain --- Sources/App/Logger/JSONLogger.swift | 1 + Sources/Bot/Services/DispatchService.swift | 17 ++++++----- Sources/Bot/Services/MergeService.swift | 28 ++++++++++++++++--- .../DispatchServiceQueueStates.swift | 20 ++++++++++++- .../MergeService/DispatchServiceTests.swift | 11 ++++---- .../MergeService/MergeServiceTests.swift | 6 ++-- Tests/BotTests/XCTestManifests.swift | 2 +- 7 files changed, 64 insertions(+), 21 deletions(-) diff --git a/Sources/App/Logger/JSONLogger.swift b/Sources/App/Logger/JSONLogger.swift index c4d9071..1f3b8f7 100644 --- a/Sources/App/Logger/JSONLogger.swift +++ b/Sources/App/Logger/JSONLogger.swift @@ -52,6 +52,7 @@ extension JSONLogger { let df = DateFormatter() df.dateFormat = "yyyy-MM-dd'T'hh:mm:ss.SSSZ" df.locale = Locale(identifier: "en_US_POSIX") + df.timeZone = TimeZone(secondsFromGMT: 0) return df }() diff --git a/Sources/Bot/Services/DispatchService.swift b/Sources/Bot/Services/DispatchService.swift index 3ddf39a..bf5ff54 100644 --- a/Sources/Bot/Services/DispatchService.swift +++ b/Sources/Bot/Services/DispatchService.swift @@ -162,13 +162,16 @@ extension DispatchService { guard !currentMergeServices.isEmpty else { return "No PR pending, all queues empty." } - return currentMergeServices.map { (entry: (key: String, value: MergeService)) -> String in - """ - ## Merge Queue for target branch: \(entry.key) ## - - \(entry.value.state.value) - """ - }.joined(separator: "\n\n") + return currentMergeServices + .sorted { $0.key < $1.key } + .map { (entry: (key: String, value: MergeService)) -> String in + """ + ## Merge Queue for target branch: \(entry.key) ## + + \(entry.value.state.value) + """ + } + .joined(separator: "\n\n") } public var queueStates: [MergeService.State] { diff --git a/Sources/Bot/Services/MergeService.swift b/Sources/Bot/Services/MergeService.swift index f7448ff..21e814c 100644 --- a/Sources/Bot/Services/MergeService.swift +++ b/Sources/Bot/Services/MergeService.swift @@ -342,8 +342,9 @@ extension MergeService { ) .flatMapError { _ in .empty } } else { + let zws = "\u{200B}" // Zero-width space character. Used so that GitHub doesn't transform `#n` into a link to Pull Request n return github.postComment( - "Your pull request was accepted and it's currently `#\(index + 1)` in the `\(current.targetBranch)` queue, hold tight ⏳", + "Your pull request was accepted and it's currently #\(zws)\(index + 1) in the `\(current.targetBranch)` queue, hold tight ⏳", in: pullRequest ) .flatMapError { _ in .empty } @@ -802,21 +803,40 @@ extension MergeService: CustomStringConvertible { extension MergeService.State: CustomStringConvertible { + private func shortPRDescription(_ pullRequest: PullRequest) -> String { + return "PR #\(pullRequest.number) (\(pullRequest.source.ref))" + } + private var shortStatusDescription: String { + switch status { + case .starting: + return "starting" + case .idle: + return "idle" + case .ready: + return "ready" + case .integrating(let pr): + return "integrating \(shortPRDescription(pr.reference))" + case .runningStatusChecks(let pr): + return "running status checks on \(shortPRDescription(pr.reference))" + case .integrationFailed(let pr, let failure): + return "integration failed on \(shortPRDescription(pr.reference)): \(failure))" + } + } private var queueDescription: String { guard pullRequests.isEmpty == false else { return "[]" } - let pullRequestsSeparator = "\n\t\t" + let pullRequestsSeparator = "\n " let pullRequestsRepresentation = pullRequests.enumerated().map { index, pullRequest in let isTP = pullRequest.isLabelled(withOneOf: self.topPriorityLabels) - return "#\(index + 1): \(pullRequest) \(isTP ? "[TP]" : "")" + return "\(index + 1). \(shortPRDescription(pullRequest))\(isTP ? " [TP]" : "")" }.joined(separator: pullRequestsSeparator) return "\(pullRequestsSeparator)\(pullRequestsRepresentation)" } public var description: String { - return "State(\n - status: \(status),\n - queue: \(queueDescription)\n)" + return "State(\n - status: \(shortStatusDescription),\n - queue: \(queueDescription)\n)" } } diff --git a/Tests/BotTests/Canned Data/DispatchServiceQueueStates.swift b/Tests/BotTests/Canned Data/DispatchServiceQueueStates.swift index baf96e4..8c35a49 100644 --- a/Tests/BotTests/Canned Data/DispatchServiceQueueStates.swift +++ b/Tests/BotTests/Canned Data/DispatchServiceQueueStates.swift @@ -1,6 +1,6 @@ import Foundation -let DispatchServiceQueueStates: Data = """ +let DispatchServiceQueueStatesJSON: Data = """ [ { "targetBranch" : "branch1", @@ -89,3 +89,21 @@ let DispatchServiceQueueStates: Data = """ } ] """.data(using: .utf8)! + + +let DispatchServiceQueueStatesString = """ +## Merge Queue for target branch: branch1 ## + +State( + - status: integrating PR #1 (some-branch), + - queue:\(" ") + 1. PR #2 (abcdef) +) + +## Merge Queue for target branch: branch2 ## + +State( + - status: integrating PR #3 (abcdef), + - queue: [] +) +""" diff --git a/Tests/BotTests/MergeService/DispatchServiceTests.swift b/Tests/BotTests/MergeService/DispatchServiceTests.swift index bf2f3ab..02ddb19 100644 --- a/Tests/BotTests/MergeService/DispatchServiceTests.swift +++ b/Tests/BotTests/MergeService/DispatchServiceTests.swift @@ -71,7 +71,7 @@ class DispatchServiceTests: XCTestCase { return .success }, - .postComment(checkComment(2, "Your pull request was accepted and it's currently `#1` in the `develop` queue, hold tight ⏳")), + .postComment(checkComment(2, "Your pull request was accepted and it's currently #\u{200B}1 in the `develop` queue, hold tight ⏳")), .getPullRequest(checkReturnPR(rel3)), .postComment(checkComment(3, "Your pull request was accepted and is going to be handled right away 🏎")), .mergePullRequest(checkPRNumber(3)), @@ -158,7 +158,7 @@ class DispatchServiceTests: XCTestCase { return .success }, - .postComment(checkComment(2, "Your pull request was accepted and it's currently `#1` in the `develop` queue, hold tight ⏳")), + .postComment(checkComment(2, "Your pull request was accepted and it's currently #\u{200B}1 in the `develop` queue, hold tight ⏳")), // Note that here we shouldn't have any API call for PR#3 since it doesn't have the integration label @@ -345,7 +345,7 @@ class DispatchServiceTests: XCTestCase { ) } - func test_json_queue_description() throws { + func test_queue_description() throws { let (branch1, branch2) = ("branch1", "branch2") let pr1 = PullRequestMetadata.stub(number: 1, headRef: MergeServiceFixture.defaultBranch, baseRef: branch1, labels: [LabelFixture.integrationLabel], mergeState: .behind) let pr2 = PullRequestMetadata.stub(number: 2, baseRef: branch1, labels: [LabelFixture.integrationLabel], mergeState: .clean) @@ -356,7 +356,7 @@ class DispatchServiceTests: XCTestCase { .getPullRequest(checkReturnPR(pr1)), .postComment(checkComment(1, "Your pull request was accepted and is going to be handled right away 🏎")), .mergeIntoBranch { _, _ in .success }, - .postComment(checkComment(2, "Your pull request was accepted and it's currently `#1` in the `branch1` queue, hold tight ⏳")), + .postComment(checkComment(2, "Your pull request was accepted and it's currently #\u{200B}1 in the `branch1` queue, hold tight ⏳")), .getPullRequest(checkReturnPR(pr3)), .postComment(checkComment(3, "Your pull request was accepted and is going to be handled right away 🏎")), .mergeIntoBranch { _, _ in .success }, @@ -382,7 +382,8 @@ class DispatchServiceTests: XCTestCase { scheduler.advance() let jsonData = try JSONEncoder().encode(dispatchServiceContext.dispatchService.queueStates) - XCTAssertEqualJSON(jsonData, DispatchServiceQueueStates) + XCTAssertEqualJSON(jsonData, DispatchServiceQueueStatesJSON) + XCTAssertEqual(dispatchServiceContext.dispatchService.queuesDescription, DispatchServiceQueueStatesString) } } diff --git a/Tests/BotTests/MergeService/MergeServiceTests.swift b/Tests/BotTests/MergeService/MergeServiceTests.swift index cc34eee..6c34986 100644 --- a/Tests/BotTests/MergeService/MergeServiceTests.swift +++ b/Tests/BotTests/MergeService/MergeServiceTests.swift @@ -333,7 +333,7 @@ class MergeServiceTests: XCTestCase { .postComment { _, _ in }, .mergeIntoBranch { _, _ in .success }, .postComment { message, pullRequest in - expect(message) == "Your pull request was accepted and it's currently `#1` in the `master` queue, hold tight ⏳" + expect(message) == "Your pull request was accepted and it's currently #\u{200B}1 in the `master` queue, hold tight ⏳" expect(pullRequest.number) == 2 }, .getPullRequest { _ in first.with(mergeState: .clean) }, @@ -914,11 +914,11 @@ class MergeServiceTests: XCTestCase { expect(pullRequest.number) == 144 }, .postComment { message, pullRequest in - expect(message) == "Your pull request was accepted and it's currently `#2` in the `master` queue, hold tight ⏳" + expect(message) == "Your pull request was accepted and it's currently #\u{200B}2 in the `master` queue, hold tight ⏳" expect(pullRequest.number) == 233 }, .postComment { message, pullRequest in - expect(message) == "Your pull request was accepted and it's currently `#3` in the `master` queue, hold tight ⏳" + expect(message) == "Your pull request was accepted and it's currently #\u{200B}3 in the `master` queue, hold tight ⏳" expect(pullRequest.number) == 377 }, .mergePullRequest { _ in }, diff --git a/Tests/BotTests/XCTestManifests.swift b/Tests/BotTests/XCTestManifests.swift index a77cbf8..279ecc6 100644 --- a/Tests/BotTests/XCTestManifests.swift +++ b/Tests/BotTests/XCTestManifests.swift @@ -8,11 +8,11 @@ extension DispatchServiceTests { static let __allTests__DispatchServiceTests = [ ("test_adding_new_pull_requests_during_integration", test_adding_new_pull_requests_during_integration), ("test_creating_new_pull_requests_to_new_target_branch_without_label", test_creating_new_pull_requests_to_new_target_branch_without_label), - ("test_json_queue_description", test_json_queue_description), ("test_mergeservice_destroyed_if_idle_long_enough", test_mergeservice_destroyed_if_idle_long_enough), ("test_mergeservice_destroyed_when_idle_after_boot", test_mergeservice_destroyed_when_idle_after_boot), ("test_mergeservice_not_destroyed_if_not_idle_long_enough", test_mergeservice_not_destroyed_if_not_idle_long_enough), ("test_multiple_pull_requests_with_different_target_branches", test_multiple_pull_requests_with_different_target_branches), + ("test_queue_description", test_queue_description), ] }