[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

@@ -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
         }()
 

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] {

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)"
     }
 }
 

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: []
+)
+"""

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)
     }
 }
 

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 },

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),
     ]
 }