FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

THE DIFFERENTIAL: legacy/modern parity held to the declared allow-list + forced-modern CI leg (#391, Tasks 12–13) by tylervick · Pull Request #450 · XCTestHTMLReport/XCTestHTMLReport · GitHub

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension .json  (1) .md  (2) .swift  (20) .yml  (1) All 4 file types selected
Only manifest files
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
10 changes: 10 additions & 0 deletions .github/workflows/test.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@ permissions:
jobs:
test:
runs-on: macos-latest
strategy:
fail-fast: false
matrix:
# `auto` is what users get. The forced `modern` leg exercises the path
# that becomes the only path once Apple removes the legacy commands —
# without it, the modern reader is only ever covered by the
# differential tests, and never end to end.
result_reader: [auto, modern]
env:
XCHR_RESULT_READER: ${{ matrix.result_reader }}
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
Expand Down
1 change: 1 addition & 0 deletions Package.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ let package = Package(
.process("Resources/TestResults.xcresult"),
.process("Resources/RetryResults.xcresult"),
.process("Resources/SanityResults.xcresult"),
.process("Resources/differential-allowlist.json"),
]
),
]
Expand Down
4 changes: 2 additions & 2 deletions Sources/XCTestHTMLReport/XCTestHtmlReport.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ struct SummaryOptions: ParsableArguments {
@Option(
name: .long,
help: ArgumentHelp(
"Which result reader to use: auto, legacy, or modern. Defaults to auto, which prefers the legacy reader while the toolchain still supports it."
"Which result reader to use: auto, legacy, or modern. Defaults to $XCHR_RESULT_READER if set, else auto, which prefers the legacy reader while the toolchain supports it."
)
)
var resultReader: ResultBackend = .auto
var resultReader: ResultBackend = .fromEnvironment()
}

@main
Expand Down
13 changes: 12 additions & 1 deletion Sources/XCTestHTMLReportCore/Classes/Models/Run.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,18 @@ struct Run: HTML {
if let logReference = run.logReference {
logContent = file.exportLogsContent(
reference: logReference,
renderingMode: renderingMode
renderingMode: renderingMode,
// Named from the run's identifier path, not from `reference`:
// the reference is backend-internal (a CAS id on legacy, a
// `--type` selector on modern), so a file named after it can
// never agree across backends. The path digest is the same
// scheme every element id already uses (#430), identical on
// both backends, and unique per run — a multi-action bundle
// gets one log file per action instead of a shared name that
// would leave last-writer-wins. No fixture exercises multiple
// actions, so that property is asserted here rather than in a
// test.
fileName: "\(identifierPath.identifier).log"
)
} else {
Logger.warning("Can't find log reference for run \(run.destination.displayName)")
Expand Down
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public struct Summary {
downsizeImagesEnabled: Bool,
downsizeScaleFactor: CGFloat,
faultCollector: FaultCollector = FaultCollector(),
backend: ResultBackend = .auto
backend: ResultBackend = .fromEnvironment()
) {
var runs: [Run] = []
var resultFiles: [ResultFile] = []
Expand Down
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ struct LegacyResultReader: ResultReader {
identifier: identifier,
// Legacy has no counterpart to Swift Testing's Arguments nodes.
arguments: [],
iterations: iterations
iterations: Self.mergingArgumentExecutions(iterations)
))
}

Expand Down Expand Up @@ -118,23 +118,36 @@ struct LegacyResultReader: ResultReader {
// ActionTestActivitySummary, so failure summaries are interleaved by
// start time. When failing sub-activities are already present we are
// on an older tool and must not add them twice.
let combined: [ParsedActivity]
let failures: [ParsedActivity]
if activities.contains(where: hasFailure) {
combined = activities
failures = []
} else {
let failures = summary.failureSummaries.map(parseFailure)
// Ordered by `start`, which replaced `finish` as the ordering key
// under decision 1 — the modern format publishes no finish, so
// `start` is the only key both backends share. This sort is not
// cosmetic: it interleaves assertion-failure rows among the
// activities so a failure renders *where it occurred* rather than
// after everything else. Dropping it — rather than re-keying it —
// would silently append every failure row at the end of the test.
combined = (activities + failures).sorted {
($0.start ?? .distantPast) < ($1.start ?? .distantPast)
}
failures = summary.failureSummaries.map(parseFailure)
}

// The skip reason lives on its own summary object, which this reader
// previously never read — the modern reader has always surfaced the
// equivalent `Failure Message` node, so skipping it here was a reader
// gap, not a format limitation. Same shape as the modern append path:
// a failure row with no timestamp, which the shared interleaving
// orders after the timeline.
let skipNotice: [ParsedActivity] = (summary.skipNoticeSummary?.message).map {
[ParsedActivity(
title: $0, isFailure: true, start: nil, attachments: [], subActivities: []
)]
} ?? []

// The interleave — `start`-keyed, per decision 1 — is not cosmetic:
// it renders each failure row *where it occurred* rather than after
// everything else. It lives in the port
// (`ParsedActivity.interleavingFailureRows`) because both readers
// must place these rows identically, and one shared function is the
// only ordering rule that cannot drift.
let combined = ParsedActivity.interleavingFailureRows(
activities: activities,
failureRows: failures + skipNotice
)

return ParsedIteration(
iterationNumber: summary.repetitionPolicySummary?.iteration,
status: Self.status(metadata.testStatus),
Expand All @@ -143,6 +156,51 @@ struct LegacyResultReader: ResultReader {
)
}

/// Collapses Swift Testing argument executions into one iteration.
///
/// A parameterized `@Test(arguments:)` reaches the legacy format as
/// duplicate sibling metadata entries sharing one identifier — the same
/// encoding as retries, except none of them carries a
/// `repetitionPolicySummary`. Rendering them as repetitions invents retry
/// semantics for what are argument variations ("3 succeeded", three
/// "Iteration 0" rows), and the modern format renders the same test as a
/// single case with `Arguments` children (answer 6). Merging on the
/// absence of repetition metadata makes both backends agree by
/// construction: durations sum (the rule answer 8 already sets), and
/// activities concatenate in source order.
///
/// True retries are untouched: every `-retry-tests-on-failure` repetition
/// carries the policy summary, so at least one iteration has a number and
/// the merge does not fire. `RetryResults` pins that in the fixture suite.
static func mergingArgumentExecutions(
_ iterations: [ParsedIteration]
) -> [ParsedIteration] {
guard iterations.count > 1,
iterations.allSatisfy({ $0.iterationNumber == nil })
else {
return iterations
}
let statuses = Set(iterations.map(\.status))
let merged: ParsedStatus
if statuses.count == 1 {
merged = statuses.first ?? .unknown
} else if statuses.contains(.failed) {
// Mirrors the modern parent node's own summary: passed only when
// every argument passed.
merged = .failed
} else if statuses.contains(.skipped) {
merged = .skipped
} else {
merged = .unknown
}
return [ParsedIteration(
iterationNumber: nil,
status: merged,
duration: iterations.reduce(0) { $0 + $1.duration },
activities: iterations.flatMap(\.activities)
)]
}

/// Legacy spellings into the neutral enum. The modern reader has the
/// mirror of this; neither emits the other's vocabulary.
static func status(_ raw: String) -> ParsedStatus {
Expand Down Expand Up @@ -188,16 +246,24 @@ struct LegacyResultReader: ResultReader {
}

private func parseAttachment(_ attachment: ActionTestAttachment) -> ParsedAttachment {
ParsedAttachment(
// The port carries no UTI (answer 4). Map it down to an extension
// here so both backends type attachments identically instead of
// the difference being allow-listed.
let ext = Self.filenameExtension(
forUTI: attachment.uniformTypeIdentifier,
filename: attachment.filename
)
return ParsedAttachment(
name: attachment.name,
filename: attachment.filename,
// The port carries no UTI (answer 4). Map it down to an extension
// here so both backends type attachments identically instead of
// the difference being allow-listed.
filenameExtension: Self.filenameExtension(
forUTI: attachment.uniformTypeIdentifier,
filename: attachment.filename
),
// Content-addressed, not `attachment.filename`: the legacy pretty
// name embeds a legacy-only uuid the modern format cannot see, so
// it can never agree across backends. The payload id can — see
// `ParsedAttachment.exportFileName`. An attachment with no payload
// has nothing to export and gets no filename, on both backends.
filename: attachment.payloadRef.map {
ParsedAttachment.exportFileName(payloadId: $0.id, filenameExtension: ext)
},
filenameExtension: ext,
payloadReference: attachment.payloadRef?.id
)
}
Expand Down
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,12 @@ extension ResultFile: PayloadProviding {
}
}

func exportLogs(reference: String) -> URL? {
func exportLogs(reference: String, fileName: String) -> URL? {
guard let logSection = file.getLogs(id: reference) else {
Logger.warning("Can't get logs with id \(reference)")
faultCollector.record(.logExportFailed, "log id \(reference)")
return nil
}
let fileName = "\(reference).log"
let url = url.appendingPathComponent(fileName)
let fileManager = FileManager.default
do {
Expand Down
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,23 @@ final class ModernPayloadStore: PayloadProviding {
}
let resolved = fileName ?? source.lastPathComponent
let destination = url.appendingPathComponent(resolved)
// Destinations are content-addressed (`ParsedAttachment.exportFileName`
// names them by payload id), so a file already at the destination *is*
// this payload and the export is idempotent: never remove, never
// rewrite. The predecessor removed-then-copied, which under Xcode
// 26.2's shared screen-recording display names raced concurrent
// exports on one path and intermittently recorded a spurious
// `.payloadExportFailed` (#449).
if FileManager.default.fileExists(atPath: destination.path) {
return relativeURL.appendingPathComponent(resolved)
}
do {
try? FileManager.default.removeItem(at: destination)
try FileManager.default.copyItem(at: source, to: destination)
return relativeURL.appendingPathComponent(resolved)
} catch {
Comment on lines +57 to 70

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Distinguish a destination this copy created from one another writer created.

The new short circuit removes the previous remove-before-copy step. FileManager.copyItem can fail after it creates the destination and writes part of the payload. The catch arm then finds the destination and reports success, so the report links a truncated attachment and the CLI still exits 0. The next export of the same name also short circuits at Line 64, so the truncated file is never repaired.

Record whether the destination existed before the copy, and delete a destination that only this failed copy created.

🛡️ Proposed guard for the failed-copy case
         if FileManager.default.fileExists(atPath: destination.path) {
             return relativeURL.appendingPathComponent(resolved)
         }
         do {
             try FileManager.default.copyItem(at: source, to: destination)
             return relativeURL.appendingPathComponent(resolved)
         } catch {
             // A concurrent writer of the same payload can still beat us to the
             // creation; its bytes are our bytes, so losing that race is
             // success, not degradation.
+            // A destination this failed copy created itself is partial, not a
+            // rival's complete payload, so it must not be trusted.
+            if let sourceSize = try? FileManager.default
+                .attributesOfItem(atPath: source.path)[.size] as? Int,
+                let destinationSize = try? FileManager.default
+                .attributesOfItem(atPath: destination.path)[.size] as? Int,
+                sourceSize != destinationSize
+            {
+                try? FileManager.default.removeItem(at: destination)
+            }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernPayloadStore.swift`
around lines 57 - 70, Update the export logic around the destination existence
check and FileManager.copyItem to record whether the destination existed before
this copy began. In the catch path, remove the destination only when it was
absent beforehand, then propagate the copy failure; preserve pre-existing
destinations without deleting or rewriting them and keep successful exports
unchanged.

// Another writer may have raced us to the destination (a second
// xchtmlreport over the same bundle): if the payload is sitting
// there, it was exported, and losing the race is not degradation.
// A concurrent writer of the same payload can still beat us to the
// creation; its bytes are our bytes, so losing that race is
// success, not degradation.
if FileManager.default.fileExists(atPath: destination.path) {
return relativeURL.appendingPathComponent(resolved)
}
Expand All @@ -87,11 +96,10 @@ final class ModernPayloadStore: PayloadProviding {
}
}

func exportLogs(reference: String) -> URL? {
func exportLogs(reference: String, fileName: String) -> URL? {
guard let text = logText(reference: reference) else {
return nil
}
let fileName = "\(reference).log"
let destination = url.appendingPathComponent(fileName)
do {
try? FileManager.default.removeItem(at: destination)
Expand Down
Loading
Loading

Back | FazBrowse Home | New Git URL