From 59cdb3b4cccdcc8bf160168b69493e182ecf4400 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Tue, 29 Oct 2024 09:16:52 -0300 Subject: [PATCH] Add a script to show spec coverage This will help us figure out what we still need to implement. Resolves #46. --- .github/workflows/check.yaml | 16 + .../xcshareddata/swiftpm/Package.resolved | 11 +- CONTRIBUTING.md | 2 +- Package.resolved | 11 +- Package.swift | 8 + Sources/BuildTool/BuildTool.swift | 435 ++++++++++++++++++ 6 files changed, 480 insertions(+), 3 deletions(-) diff --git a/.github/workflows/check.yaml b/.github/workflows/check.yaml index 5d3fad21..fe75f2f7 100644 --- a/.github/workflows/check.yaml +++ b/.github/workflows/check.yaml @@ -38,6 +38,21 @@ jobs: - run: swift run BuildTool lint + spec-coverage: + runs-on: macos-latest + steps: + - uses: actions/checkout@v4 + + # This step can be removed once the runners’ default version of Xcode is 16 or above + - uses: maxim-lobanov/setup-xcode@v1 + with: + xcode-version: 16 + + - name: Spec coverage + run: swift run BuildTool spec-coverage + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + generate-matrices: runs-on: macos-latest outputs: @@ -116,6 +131,7 @@ jobs: runs-on: ubuntu-latest needs: - lint + - spec-coverage - check-spm - check-xcode - check-example-app diff --git a/AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved b/AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved index 7961d42a..9bf42bea 100644 --- a/AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved +++ b/AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "4a28f4c041628961c5cac754904f5f76c33935bbc8417b2e036e0ab6c33b7a0a", + "originHash" : "a296396707b7685153f4cf548f6281f483d562002fe11235f1fc3bb053be91d7", "pins" : [ { "identity" : "ably-cocoa", @@ -54,6 +54,15 @@ "revision" : "3d2dc41a01f9e49d84f0a3925fb858bed64f702d", "version" : "1.1.2" } + }, + { + "identity" : "table", + "kind" : "remoteSourceControl", + "location" : "https://github.com/JanGorman/Table.git", + "state" : { + "revision" : "7b8521c3b1078ba0b347964aec7c503cd6f1ff40", + "version" : "1.1.1" + } } ], "version" : 3 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8166d0b5..794ada92 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -85,7 +85,7 @@ func test3 { … } func test4 { … } ``` -In [#46](https://github.com/ably-labs/ably-chat-swift/issues/46), we’ll write a script that uses these tags to generate a report about how much of the feature spec we’ve implemented. +You can run `swift run BuildTool spec-coverage` to generate a report about how many spec points have been implemented and/or tested. This script is also run in CI by the `spec-coverage` job. This script will currently only detect a spec point attribution tag if it’s written exactly as shown above; that is, in a `//` comment with a single space between each component of the tag. #### Marking a spec point as untested diff --git a/Package.resolved b/Package.resolved index 33b18ea8..7926dd68 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "48d264bc362ab438d94b78732ce83d1971c3a1264d089085edd3814083479fc6", + "originHash" : "f00ee2e8c80adfe8d72deb089738cdb967aeae43e71837f90d99fc602728fe45", "pins" : [ { "identity" : "ably-cocoa", @@ -54,6 +54,15 @@ "revision" : "3d2dc41a01f9e49d84f0a3925fb858bed64f702d", "version" : "1.1.2" } + }, + { + "identity" : "table", + "kind" : "remoteSourceControl", + "location" : "https://github.com/JanGorman/Table.git", + "state" : { + "revision" : "7b8521c3b1078ba0b347964aec7c503cd6f1ff40", + "version" : "1.1.1" + } } ], "version" : 3 diff --git a/Package.swift b/Package.swift index 74824c34..f6ff655e 100644 --- a/Package.swift +++ b/Package.swift @@ -31,6 +31,10 @@ let package = Package( url: "https://github.com/apple/swift-async-algorithms", from: "1.0.1" ), + .package( + url: "https://github.com/JanGorman/Table.git", + from: "1.1.1" + ), ], targets: [ .target( @@ -67,6 +71,10 @@ let package = Package( name: "AsyncAlgorithms", package: "swift-async-algorithms" ), + .product( + name: "Table", + package: "Table" + ), ] ), ] diff --git a/Sources/BuildTool/BuildTool.swift b/Sources/BuildTool/BuildTool.swift index 5c451a10..bfe389c9 100644 --- a/Sources/BuildTool/BuildTool.swift +++ b/Sources/BuildTool/BuildTool.swift @@ -1,5 +1,7 @@ import ArgumentParser +import AsyncAlgorithms import Foundation +import Table @main @available(macOS 14, *) @@ -10,6 +12,7 @@ struct BuildTool: AsyncParsableCommand { BuildExampleApp.self, GenerateMatrices.self, Lint.self, + SpecCoverage.self, ] ) } @@ -196,3 +199,435 @@ struct Lint: AsyncParsableCommand { return try String(data: data, encoding: .utf8) } } + +@available(macOS 14, *) +struct SpecCoverage: AsyncParsableCommand { + static let configuration = CommandConfiguration( + abstract: "Print information about which spec points are implemented", + discussion: "You can set the GITHUB_TOKEN environment variable to provide a GitHub authentication token to use when fetching the latest commit." + ) + + enum Error: Swift.Error { + case unexpectedStatusCodeLoadingCommit(Int) + case unexpectedStatusCodeLoadingSpec(Int) + case conformanceToNonexistentSpecPoints(specPointIDs: [String]) + case couldNotFindTestTarget + case malformedSpecOneOfTag + case specUntestedTagMissingComment + } + + /** + * A representation of the chat features spec Textile file. + */ + private struct SpecFile { + struct SpecPoint: Identifiable { + var id: String + var isTestable: Bool + + init?(specLine: String) { + // example line that corresponds to a testable spec point: + // ** @(CHA-RS4b)@ @[Testable]@ Room status update events must contain the previous room status. + // (This `Testable` is a convention that’s being used only in the Chat spec) + + let specPointLineRegex = /^\*+ @\((.*?)\)@( @\[Testable\]@ )?/ + + // swiftlint:disable:next force_try + guard let match = try! specPointLineRegex.firstMatch(in: specLine) else { + return nil + } + + id = String(match.output.1) + isTestable = match.output.2 != nil + } + } + + var specPoints: [SpecPoint] + + init(fileContents: String) { + specPoints = fileContents.split(whereSeparator: \.isNewline).compactMap { line in + SpecPoint(specLine: String(line)) + } + } + } + + /** + * A tag, extracted from a comment in the SDK’s test code, which indicates conformance to a spec point, as described in the "Attributing tests to a spec point" section of `CONTRIBUTING.md`. + */ + private struct ConformanceTag { + enum `Type` { + case spec(comment: String?) + case specOneOf(index: Int, total: Int, comment: String?) + case specPartial(comment: String?) + case specUntested(comment: String) + } + + var type: `Type` + var specPointID: String + + init?(sourceLine: String) throws { + let conformanceTagSourceLineRegex = /^\s+\/\/ @spec(OneOf|Partial|Untested)?(?:\((\d)?\/(\d)?\))? (.*?)(?: - (.*))?$/ + + guard let match = try conformanceTagSourceLineRegex.firstMatch(in: sourceLine) else { + return nil + } + + specPointID = String(match.output.4) + + let comment: String? = if let capture = match.output.5 { + String(capture) + } else { + nil + } + + switch match.output.1 { + case nil: + type = .spec(comment: comment) + case "OneOf": + guard let indexString = match.output.2, let index = Int(indexString), let totalString = match.output.3, let total = Int(totalString) else { + throw Error.malformedSpecOneOfTag + } + type = .specOneOf(index: index, total: total, comment: comment) + case "Partial": + type = .specPartial(comment: comment) + case "Untested": + guard let comment else { + throw Error.specUntestedTagMissingComment + } + type = .specUntested(comment: comment) + default: + preconditionFailure("Incorrect assumption when reading regex captures") + } + } + } + + private struct CoverageReport { + struct Summary { + var specPointCount: Int + var testableSpecPointCount: Int + private var specPointCountsByCoverageLevel: [CoverageLevel: Int] + + init(specPointCount: Int, testableSpecPointCount: Int, specPointCoverages: [SpecPointCoverage]) { + self.specPointCount = specPointCount + self.testableSpecPointCount = testableSpecPointCount + + specPointCountsByCoverageLevel = Dictionary(grouping: specPointCoverages, by: \.coverageLevel) + .mapValues(\.count) + for coverageLevel in CoverageLevel.allCases where specPointCountsByCoverageLevel[coverageLevel] == nil { + specPointCountsByCoverageLevel[coverageLevel] = 0 + } + } + + func specPointCountForCoverageLevel(_ coverageLevel: CoverageLevel) -> Int { + guard let count = specPointCountsByCoverageLevel[coverageLevel] else { + preconditionFailure("Missing key \(coverageLevel)") + } + return count + } + } + + var summary: Summary + + /** + * One per testable spec point. + */ + var testableSpecPointCoverages: [SpecPointCoverage] + + /** + * The IDs of spec points that are not marked as Testable but which have a conformance tag. We’ll emit a warning for these, because it might mean that the spec point they refer to has been replaced or deleted; might need to re-think this approach if it turns out there are other good reasons for testing non-testable points). + */ + var nonTestableSpecPointIDsWithConformanceTags: Set + + enum CoverageLevel: CaseIterable { + case tested + case partiallyTested + case implementedButDeliberatelyNotTested + case notTested + } + + struct SpecPointCoverage { + var specPointID: String + var coverageLevel: CoverageLevel + var comments: [String] + } + + static func generate(specFile: SpecFile, conformanceTags: [ConformanceTag]) throws -> CoverageReport { + let conformanceTagsBySpecPointID = Dictionary(grouping: conformanceTags, by: \.specPointID) + + // 1. Check that all of the conformance tags correspond to actual spec points. + let invalidSpecPointIDs = Set(conformanceTagsBySpecPointID.keys).subtracting(specFile.specPoints.map(\.id)) + if !invalidSpecPointIDs.isEmpty { + throw Error.conformanceToNonexistentSpecPoints(specPointIDs: invalidSpecPointIDs.sorted()) + } + + // 2. Find any conformance tags for non-testable spec points (see documentation of the `nonTestableSpecPointIDsWithConformanceTags` property) for motivation. + let specPointsByID = Dictionary(grouping: specFile.specPoints, by: \.id) + + var nonTestableSpecPointIDsWithConformanceTags: Set = [] + for conformanceTag in conformanceTags { + let specPointID = conformanceTag.specPointID + let specPoint = specPointsByID[specPointID]!.first! + if !specPoint.isTestable { + nonTestableSpecPointIDsWithConformanceTags.insert(specPointID) + } + } + + // 3. Determine the coverage of each testable spec point. + let testableSpecPoints = specFile.specPoints.filter(\.isTestable) + let specPointCoverages = testableSpecPoints.map { specPoint in + var coverageLevel: CoverageLevel? + var comments: [String] = [] + + let conformanceTagsForSpecPoint = conformanceTagsBySpecPointID[specPoint.id, default: []] + // TODO: https://github.com/ably-labs/ably-chat-swift/issues/96 - check for contradictory tags, validate the specOneOf(m, n) tags + for conformanceTag in conformanceTagsForSpecPoint { + // We only make use of the comments that explain why something is untested or partially tested. + switch conformanceTag.type { + case .spec: + coverageLevel = .tested + case .specOneOf: + coverageLevel = .tested + case let .specPartial(comment: comment): + coverageLevel = .partiallyTested + if let comment { + comments.append(comment) + } + case let .specUntested(comment: comment): + coverageLevel = .implementedButDeliberatelyNotTested + comments.append(comment) + } + } + + return SpecPointCoverage( + specPointID: specPoint.id, + coverageLevel: coverageLevel ?? .notTested, + comments: comments + ) + } + + return .init( + summary: .init( + specPointCount: specFile.specPoints.count, + testableSpecPointCount: testableSpecPoints.count, + specPointCoverages: specPointCoverages + ), + testableSpecPointCoverages: specPointCoverages, + nonTestableSpecPointIDsWithConformanceTags: nonTestableSpecPointIDsWithConformanceTags + ) + } + } + + private struct CoverageReportViewModel { + struct SummaryViewModel { + var specContentsMessage: String + var table: String + + init(summary: CoverageReport.Summary) { + specContentsMessage = "There are \(summary.specPointCount) spec points, \(summary.testableSpecPointCount) of which are marked as testable." + + let headers = ["Coverage level", "Number of spec points", "Percentage of testable spec points"] + + let percentageFormatter = NumberFormatter() + percentageFormatter.numberStyle = .percent + percentageFormatter.minimumFractionDigits = 1 + percentageFormatter.maximumFractionDigits = 1 + + let rows = CoverageReport.CoverageLevel.allCases.map { coverageLevel in + let specPointCount = summary.specPointCountForCoverageLevel(coverageLevel) + + return [ + CoverageReportViewModel.descriptionForCoverageLevel(coverageLevel), + String(specPointCount), + percentageFormatter.string(from: NSNumber(value: Double(specPointCount) / Double(summary.testableSpecPointCount)))!, + ] + } + + // swiftlint:disable:next force_try + table = try! Table(data: [headers] + rows).table() + } + } + + var summary: SummaryViewModel + var warningMessages: [String] + var specPointsTable: String + + init(report: CoverageReport) { + warningMessages = [] + if !report.nonTestableSpecPointIDsWithConformanceTags.isEmpty { + warningMessages.append("Warning: The tests have conformance tags for the following non-Testable spec points: \(Array(report.nonTestableSpecPointIDsWithConformanceTags).sorted().joined(separator: ", ")). Have these spec points been deleted or replaced?") + } + + let headers = ["Spec point ID", "Coverage level", "Comments"] + + let rows = report.testableSpecPointCoverages.map { coverage in + // TODO: https://github.com/ably-labs/ably-chat-swift/issues/94 - Improve the output of comments. The Table library doesn’t: + // + // 1. offer the ability to wrap long lines + // 2. handle multi-line strings + // + // so I’m currently just combining all the comments into a single line and then truncating this line. + let comments = coverage.comments.joined(separator: ",") + + let truncateCommentsToLength = 80 + let truncatedComments = comments.count > truncateCommentsToLength ? comments.prefix(truncateCommentsToLength - 1) + "…" : comments + + return [ + coverage.specPointID, + Self.descriptionForCoverageLevel(coverage.coverageLevel), + truncatedComments, + ] + } + + // swiftlint:disable:next force_try + specPointsTable = try! Table(data: [headers] + rows).table() + + summary = .init(summary: report.summary) + } + + static func descriptionForCoverageLevel(_ coverageLevel: CoverageReport.CoverageLevel) -> String { + switch coverageLevel { + case .tested: + "Tested" + case .partiallyTested: + "Partially tested" + case .implementedButDeliberatelyNotTested: + "Implemented, not tested" + case .notTested: + "Not tested" + } + } + } + + mutating func run() async throws { + // TODO: https://github.com/ably-labs/ably-chat-swift/issues/97 - switch to use main at some point + let branchName = "chat-lifecycle" + + let commitSHA = try await fetchLatestSpecCommitSHAForBranchName(branchName) + print("Using latest spec commit (\(commitSHA.prefix(7))) from branch \(branchName).\n") + + let specFile = try await loadSpecFile(forCommitSHA: commitSHA) + let conformanceTags = try await fetchConformanceTags() + + let report = try CoverageReport.generate(specFile: specFile, conformanceTags: conformanceTags) + + let reportViewModel = CoverageReportViewModel(report: report) + print(reportViewModel.summary.specContentsMessage + "\n") + print((reportViewModel.warningMessages + [""]).joined(separator: "\n")) + print(reportViewModel.summary.table) + print(reportViewModel.specPointsTable) + } + + /** + * The response from GitHub’s [“get a commit” endpoint](https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#get-a-commit). + */ + private struct GitHubCommitResponseDTO: Codable { + var sha: String + } + + private func fetchLatestSpecCommitSHAForBranchName(_ branchName: String) async throws -> String { + // https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#get-a-commit + var request = URLRequest(url: URL(string: "https://api.github.com/repos/ably/specification/commits/\(branchName)")!) + request.setValue("application/vnd.github+json", forHTTPHeaderField: "Accept") + request.setValue("2022-11-28", forHTTPHeaderField: "X-GitHub-Api-Version") + if let gitHubToken = ProcessInfo.processInfo.environment["GITHUB_TOKEN"] { + print("Using GitHub token from GITHUB_TOKEN environment variable.") + request.setValue("Bearer \(gitHubToken)", forHTTPHeaderField: "Authorization") + } + + let (commitData, response) = try await URLSession.shared.data(for: request) + + guard let httpResponse = response as? HTTPURLResponse else { + preconditionFailure("Expected an HTTPURLResponse") + } + + guard (200 ..< 300).contains(httpResponse.statusCode) else { + throw Error.unexpectedStatusCodeLoadingCommit(httpResponse.statusCode) + } + + let responseDTO = try JSONDecoder().decode(GitHubCommitResponseDTO.self, from: commitData) + return responseDTO.sha + } + + private func loadSpecFile(forCommitSHA commitSHA: String) async throws -> SpecFile { + let specFileURL = URL(string: "https://raw.githubusercontent.com/ably/specification/\(commitSHA)/textile/chat-features.textile")! + let (specData, response) = try await URLSession.shared.data(from: specFileURL) + + guard let httpResponse = response as? HTTPURLResponse else { + preconditionFailure("Expected an HTTPURLResponse") + } + + guard (200 ..< 300).contains(httpResponse.statusCode) else { + throw Error.unexpectedStatusCodeLoadingSpec(httpResponse.statusCode) + } + + let specContents: String = try String(data: specData, encoding: .utf8) + + return SpecFile(fileContents: specContents) + } + + private func fetchConformanceTags() async throws -> [ConformanceTag] { + let testSourceFilePaths = try await fetchTestSourceFilePaths() + let testSources = try await withThrowingTaskGroup(of: String.self) { group in + for testSourceFilePath in testSourceFilePaths { + group.addTask { + let (data, _) = try await URLSession.shared.data(from: testSourceFilePath) + return try String(data: data, encoding: .utf8) + } + } + + return try await Array(group) + } + + return try testSources.flatMap { testSource in + try testSource.split(whereSeparator: \.isNewline).compactMap { sourceLine in + try ConformanceTag(sourceLine: String(sourceLine)) + } + } + } + + /** + * The result of invoking `swift package describe`. + */ + private struct PackageDescribeOutput: Codable { + /** + * The absolute path of the directory containing the `Package.swift` file. + */ + var path: String + + struct Target: Codable { + var name: String + + /** + * The path of this target’s sources, relative to ``PackageDescribeOutput.path``. + */ + var path: String + + /** + * The paths of each of this target’s sources, relative to ``path``. + */ + var sources: [String] + } + + var targets: [Target] + } + + /** + * Fetches the absolute file URLs of all of the source files for the SDK’s tests. + */ + private func fetchTestSourceFilePaths() async throws -> [URL] { + let packageDescribeOutputData = try await ProcessRunner.runAndReturnStdout( + executableName: "swift", + arguments: ["package", "describe", "--type", "json"] + ) + + let packageDescribeOutput = try JSONDecoder().decode(PackageDescribeOutput.self, from: packageDescribeOutputData) + + guard let testTarget = (packageDescribeOutput.targets.first { $0.name == "AblyChatTests" }) else { + throw Error.couldNotFindTestTarget + } + + let targetSourcesAbsoluteURL = URL(filePath: packageDescribeOutput.path).appending(path: testTarget.path) + return testTarget.sources.map { sourceRelativePath in + targetSourcesAbsoluteURL.appending(component: sourceRelativePath) + } + } +}