-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Coverage Converter update #25
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,57 @@ | |
import Foundation | ||
import XCResultKit | ||
|
||
enum JSONParseError: Error { | ||
case convertError(code: Int, message: String) | ||
} | ||
|
||
// Subrange information struct | ||
struct Subrange: Decodable { | ||
let column: Int | ||
let executionCount: Int | ||
let length: Int | ||
} | ||
|
||
// LineDetail information struct | ||
struct LineDetail: Decodable { | ||
let isExecutable: Bool | ||
let line: Int | ||
let executionCount: Int? | ||
let subranges: [Subrange]? | ||
} | ||
|
||
// FileCoverage information struct | ||
struct FileCoverage: Decodable { | ||
let files: [String: [LineDetail]] | ||
|
||
// Custom initializer to handle the top-level dictionary | ||
init(from decoder: Decoder) throws { | ||
let container = try decoder.container(keyedBy: CodingKeys.self) | ||
var filesDict = [String: [LineDetail]]() | ||
|
||
for key in container.allKeys { | ||
let keyString = key.stringValue | ||
let lineDetails = try container.decode([LineDetail].self, forKey: key) | ||
filesDict[keyString] = lineDetails | ||
} | ||
self.files = filesDict | ||
} | ||
|
||
private struct CodingKeys: CodingKey { | ||
var stringValue: String | ||
var intValue: Int? | ||
|
||
init?(stringValue: String) { | ||
self.stringValue = stringValue | ||
self.intValue = nil | ||
} | ||
|
||
init?(intValue: Int) { | ||
return nil | ||
} | ||
} | ||
} | ||
|
||
public class CoberturaCoverageConverter: CoverageConverter, XmlSerializable { | ||
|
||
public var xmlString: String { | ||
|
@@ -60,44 +111,55 @@ public class CoberturaCoverageConverter: CoverageConverter, XmlSerializable { | |
let packagesElement = XMLElement(name: "packages") | ||
rootElement.addChild(packagesElement) | ||
|
||
// Get the xccov results as a JSON. | ||
var arguments = ["xccov", "view"] | ||
if resultFile.url.pathExtension == "xcresult" { | ||
arguments.append("--archive") | ||
} | ||
arguments.append("--json") | ||
arguments.append(resultFile.url.path) | ||
let coverageData = try Shell.execute(program: "/usr/bin/xcrun", with: arguments) | ||
let resultsString = String(decoding: coverageData, as: UTF8.self) | ||
guard let jsonData = resultsString.data(using: String.Encoding.utf8) else { | ||
writeToStdError("Failed to convert to Data") | ||
throw JSONParseError.convertError(code: 0, message: "Failed to convert to Data object") | ||
} | ||
guard let coverageJson = try? JSONDecoder().decode(FileCoverage.self, from: jsonData) else { | ||
writeToStdError("Failed to convert to JSON") | ||
throw JSONParseError.convertError(code: 0, message: "Failed to convert to JSON Object") | ||
} | ||
|
||
let fileInfoSemaphore = DispatchSemaphore(value: 1) | ||
var fileInfo: [FileInfo] = [] | ||
|
||
// since we need to invoke xccov for each file, it takes pretty much time | ||
// so we invoke it in parallel on 8 threads, that speeds up things considerably | ||
let queue = OperationQueue() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that xcccv tool doesn't need to get called repeatedly we do not need to spread the for loop over different threads. In fact I testest and without spawning to threads it ran slightly faster. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey there, sorry for the delay here, haven't been at my computer much this weekend. Is there a way to toggle this to be a CLI flag? I imagine that for a very large results file it might be faster to work serially on a machine with not a lot of CPUs but on a machine with 8+ cpus I image it might eek out better performance? I can try and work on a PR to add that functionality if you'd be alright with that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just saving the CPU for other parallel tasks. I will try some larger xcresult bundles and measure the difference with the PR I just created and with your version. Let's see, it it is worth it to block more than one core for this. And let's take this discussion to the new PR. I invited you as collaborator and once you accept I can add you as reviewer to the PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me, I'm not all too familiar with Swift's parallelism and my best frame of reference is Go's goroutines (greenthreads) |
||
queue.maxConcurrentOperationCount = 8 //Deadlock if this is = 1 | ||
queue.maxConcurrentOperationCount = ProcessInfo.processInfo.processorCount //Deadlock if this is = 1 | ||
queue.qualityOfService = .userInitiated | ||
for (fileName, value) in coverageJson.files { | ||
let op = BlockOperation { | ||
var fileLines = [LineInfo]() // This will store information about each line. | ||
|
||
var processedFiles = [String]() | ||
for target in codeCoverage.targets { | ||
if !coverageTargets.isEmpty { | ||
guard coverageTargets.contains(target.name) else { continue } | ||
} | ||
for codeCovFile in target.files { | ||
let file = codeCovFile.path | ||
guard !processedFiles.contains(file) else { continue } | ||
processedFiles.append(file) | ||
guard !file.isEmpty else { continue } | ||
if !quiet { | ||
writeToStdError("Coverage for: \(file)\n") | ||
} | ||
let op = BlockOperation { [self] in | ||
do { | ||
let coverage = try fileCoverage(for: file, relativeTo: projectRoot) | ||
fileInfoSemaphore.wait() | ||
fileInfo.append(coverage) | ||
fileInfoSemaphore.signal() | ||
} catch { | ||
writeToStdErrorLn(error.localizedDescription) | ||
for lineData in value { | ||
let lineNum = lineData.line | ||
guard var covered = lineData.executionCount else { | ||
continue | ||
} | ||
// If the line coverage count is a MAX_INT, just set it to 1 | ||
if covered == Int.max { | ||
covered = 1 | ||
} | ||
let line = LineInfo(lineNumber: String(lineNum), coverage: covered) | ||
fileLines.append(line) | ||
} | ||
queue.addOperation(op) | ||
|
||
let fileInfoInst = FileInfo(path: self.relativePath(for: fileName, relativeTo: self.projectRoot), lines: fileLines) | ||
fileInfoSemaphore.wait() | ||
fileInfo.append(fileInfoInst) | ||
fileInfoSemaphore.signal() | ||
} | ||
queue.addOperation(op) | ||
} | ||
// This will block until all our operation have compleated (or been canceled) | ||
queue.waitUntilAllOperationsAreFinished() | ||
|
||
// Sort files to avoid duplicated packages | ||
fileInfo.sort { $0.path > $1.path } | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to convert coverageData to String and then back to Data.
Instead we can use
right away. Right?
Skip line 122 to 127