Skip to content

Commit

Permalink
Fix the naive approach to consolidating progress bars (#24)
Browse files Browse the repository at this point in the history
Change Description: These changes make it so that the progress bar consolidation is much more robust than before. Depending on the options you passed, it was possible to see progress bars duplicated. This is now eliminated by not exporting attachments until we've found all of them and determined which URLs they will be exported to. We have a dictionary that uses the URL's path as the key & add attachments that want to be exported to that URL into the corresponding value in the dictionary. We also sort the keys of the dictionary at the end so that the progress bars will be nicely ordered. Display name logic has been simplified to just be based off of the relative name as compared to the base screenshot URL so that new folder additions will automatically get displayName support.

Test Plan/Testing Performed: Tested with these changes that if I provide various options, I only see one progress bar with the same display name. When there is no relevant display name, "Exporting Screenshots" is shown. Ran with xcresults we have locally for Xcode 11 with test plan configs.
  • Loading branch information
abotkin-cpi authored Oct 16, 2019
1 parent 0b0c247 commit 3e2d6c5
Showing 1 changed file with 19 additions and 47 deletions.
66 changes: 19 additions & 47 deletions Sources/xcparse/XCPParser.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import Foundation
import SPMUtility
import XCParseCore

let xcparseCurrentVersion = Version(0, 5, 0)
let xcparseCurrentVersion = Version(0, 5, 1)

enum InteractiveModeOptionType: String {
case screenshot = "s"
Expand Down Expand Up @@ -73,29 +73,6 @@ struct AttachmentExportOptions {
var divideByTargetOS: Bool = false
var divideByTestRun: Bool = false

func displayName(withDeviceRecord deviceRecord: ActionDeviceRecord, testPlanRun: ActionTestPlanRunSummary) -> String {
var retval: String = ""

if self.divideByTargetModel == true && self.divideByTargetOS == true {
retval += deviceRecord.modelName + " (\(deviceRecord.operatingSystemVersion))"
} else if self.divideByTargetModel == true {
retval += deviceRecord.modelName
} else if self.divideByTargetOS == true {
retval += deviceRecord.operatingSystemVersion
}

if self.divideByTestRun == true {
if let testPlanRunName = testPlanRun.name {
if retval.count > 0 {
retval += " - "
}
retval += testPlanRunName
}
}

return retval
}

func baseScreenshotDirectoryURL(path: String) -> Foundation.URL {
let destinationURL = URL.init(fileURLWithPath: path)
if self.addTestScreenshotsDirectory {
Expand Down Expand Up @@ -157,9 +134,10 @@ class XCPParser {
return
}

let actions = invocationRecord.actions.filter { $0.actionResult.testsRef != nil }
// This is going to be the mapping of the places we're going to export the screenshots to
var exportURLsToAttachments: [String : [ActionTestAttachment]] = [:]

var attachmentsToExportToBaseDirectory: [ActionTestAttachment] = []
let actions = invocationRecord.actions.filter { $0.actionResult.testsRef != nil }
for action in actions {
guard let testRef = action.actionResult.testsRef else {
continue
Expand All @@ -179,7 +157,6 @@ class XCPParser {
continue
}

var attachmentsToExportToActionDirectory: [ActionTestAttachment] = []
for testPlanRun in testPlanRunSummaries.summaries {
let testPlanRunScreenshotURL = options.screenshotDirectoryURL(testPlanRun, forBaseURL: actionScreenshotDirectoryURL)
if testPlanRunScreenshotURL.createDirectoryIfNecessary() != true {
Expand All @@ -189,30 +166,25 @@ class XCPParser {
let testableSummaries = testPlanRun.testableSummaries
let testableSummariesAttachments = testableSummaries.flatMap { $0.attachments(withXCResult: xcresult) }

// Now that we know what we want to export, figure out if it should go to base directory or not
if (testPlanRunScreenshotURL == screenshotBaseDirectoryURL) {
// Wait to export these all in one nice progress bar at end
attachmentsToExportToBaseDirectory.append(contentsOf: testableSummariesAttachments)
} else if (testPlanRunScreenshotURL == actionScreenshotDirectoryURL) {
// Wait to export these all in one nice progress bar for the entire action
attachmentsToExportToActionDirectory.append(contentsOf: testableSummariesAttachments)
} else {
// Let's get ready to export!
let displayName = options.displayName(withDeviceRecord: targetDeviceRecord, testPlanRun: testPlanRun)
self.exportScreenshots(withXCResult: xcresult, toDirectory: testPlanRunScreenshotURL, attachments: testableSummariesAttachments, displayName: displayName)
}
// Now that we know what we want to export, save it to the dictionary so we can have all the exports
// done at once with one progress bar per URL
var existingAttachmentsForBaseURL = exportURLsToAttachments[testPlanRunScreenshotURL.path] ?? []
existingAttachmentsForBaseURL.append(contentsOf: testableSummariesAttachments)
exportURLsToAttachments[testPlanRunScreenshotURL.path] = existingAttachmentsForBaseURL
}
}

// Check if we deferred any screenshot export to the action's directory
if attachmentsToExportToActionDirectory.count > 0 {
let displayName = actionScreenshotDirectoryURL.lastPathComponent
self.exportScreenshots(withXCResult: xcresult, toDirectory: actionScreenshotDirectoryURL, attachments: attachmentsToExportToActionDirectory, displayName: displayName)
// Let's get ready to export!
for (exportURLString, attachmentsToExport) in exportURLsToAttachments.sorted(by: { $0.key < $1.key }) {
let exportURL = Foundation.URL(fileURLWithPath: exportURLString)
if attachmentsToExport.count <= 0 {
continue
}
}

// Now let's export everything that wanted to just be in the base directory
if attachmentsToExportToBaseDirectory.count > 0 {
self.exportScreenshots(withXCResult: xcresult, toDirectory: screenshotBaseDirectoryURL, attachments: attachmentsToExportToBaseDirectory)
let exportRelativePath = exportURL.path.replacingOccurrences(of: screenshotBaseDirectoryURL.path, with: "").trimmingCharacters(in: CharacterSet(charactersIn: "/"))
let displayName = exportRelativePath.replacingOccurrences(of: "/", with: " - ")

self.exportScreenshots(withXCResult: xcresult, toDirectory: exportURL, attachments: attachmentsToExport, displayName: displayName)
}
}

Expand Down

0 comments on commit 3e2d6c5

Please sign in to comment.