THE DIFFERENTIAL: legacy/modern parity held to the declared allow-list + forced-modern CI leg (#391, Tasks 12–13) - #450
Merged
tylervick merged 4 commits intoAug 13, 2026
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
… race Running both readers over freshly generated fixtures surfaced divergences the spec's tables did not anticipate. Each lands here as a rule, recorded in the spec's "Task 12 execution rules" section; none adds an allow-list entry. - Content-addressed attachment exports: both backends name every exported payload <payloadId>.<ext> — the one attachment identifier the two formats share. Xcode 26.2 gives every auto screen recording in a session one shared display name, so name-keyed exports collapsed distinct payloads onto one path and raced concurrent copies (#449, reproduced with a live EEXIST trace and a 1-in-12 test flake). The export is now idempotent: an existing destination is the payload, never removed. - Symbol-annotation rows (no startTime) and attachment-shadow rows (leaf, non-failure, startTime == sibling attachment timestamp) are dropped by the modern reader; both are 26.2 bookkeeping the legacy tree never had. The shadow join's guards are load-bearing: a genuine failure row shares the attachment's millisecond on testWithSpecialChars(). - Swift Testing names come from the identifier's function form on both backends; the @test display name is a field only one backend can fill. - Legacy merges parameterized argument executions (duplicate siblings with no repetitionPolicySummary) into one iteration; true retries keep their numbers. - Expected failures are non-events on both backends: messages claim and remove their exact-title activity rows instead of joining. - Skip notices were a legacy reader gap: the reason was always on skipNoticeSummary; it now renders the same appended row modern emits. - Failure-row placement goes through one shared total-ordered interleave (ParsedActivity.interleavingFailureRows) fed by both readers; the modern reader hoists failure tips (isFailure now means "is the assertion row" — tip of the flagged chain). Re-nesting legacy rows by time window was tested and rejected: windows collide at millisecond granularity and misplace rows. - Run logs export as <run-identifier-digest>.log on both backends instead of backend-internal reference names. Refs #391. Fixes #449. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…w-list Renders each fixture through both readers, forced explicitly, and asserts (Task 12): per-run counts, the identifier→status map, and exported attachment filenames AND bytes match exactly; after normalizing identifier digests (#430's [0-9a-f]{32}, .linking renders only) and masking exactly the four declared losses, the two renders are byte-identical. Skips loudly when the toolchain has no legacy commands, and proves the forced backend actually resolved rather than assuming it. Anti-rot runs in both directions: a diff outside the allow-list fails the build, and an entry that masks nothing real also fails — "fires" means omitting the rule (others still applied) leaves the renders unequal, so an entry cannot rot silently once Apple fills the gap. All four entries fire: durations on all three fixtures, wrapperGroups on all three, failureTitlePrefix and attachmentDisplayNames on TestResults and RetryResults. The masker diverged from the plan's snippets in four evidence-forced ways, recorded in the plan's Task 12 execution note: wrapperGroups is a structural SwiftSoup unwrap (line-filtering left the wrapper's div skeleton behind), the display-name anchor spans the icon line the plan's regex tripped on, line joins are canonicalized before comparing (template concatenation breaks lines differently across nesting depths), and XCTest-case duration coverage lost to the over-broad durations mask is restored by a model-level assertion. Refs #391. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
One matrix leg runs the whole suite with the modern reader forced, so the path that becomes the only path once Apple removes the legacy commands is exercised end to end on every PR — not only through the differential. The env var is the CI override, not a new control surface: it defaults both Summary.init's backend and the CLI's --result-reader (so CLI-driven tests pick it up through the spawned binary), the flag still wins when passed, and an unrecognised value degrades to auto. Verified locally: 92 tests green under both XCHR_RESULT_READER=modern and =auto. Fixture cache (#436) is shared across legs unchanged; no new action steps. Refs #391. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tems Spec: new "Task 12 execution rules (2026-08-12)" section — content-addressed exports (fixes #449), symbol-annotation and attachment-shadow drops, Swift Testing names from the identifier, parameterized-execution merge, expected failures as non-events, skip-notice fix, the shared failure-row interleave with the reversible hoist, and run-log naming — each with the evidence that forced it. The attachment-filename table row is superseded, the Tree-shape display-name paragraph becomes a model rule, and attachmentDisplayNames' Exercised-by cell gains RetryResults (measured). Plan: Task 12 execution note (where the shipped harness supersedes the snippets, and why); Task 15 gains a release-notes checklist so the 4.0 output changes accepted here cannot be missed by the docs task. Refs #391. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copy link
Copy Markdown
WalkthroughThe change adds environment-based backend selection, aligns legacy and modern result parsing, standardizes log and attachment exports, preserves existing payload files, and adds differential tests with an explicit allow-list. ChangesResult backend and export contracts
Legacy and modern reader behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟠 High · up to f1dba The PR changes result parsing and attachment export behavior, but concurrent exports can currently accept truncated files or remove an existing destination during replacement, producing successful reports with missing or corrupted attachments. Merge should be blocked until both export paths are made race-safe. Sequence Diagram(s)sequenceDiagram
participant TestMatrix
participant Summary
participant LegacyResultReader
participant ModernResultReader
participant DifferentialTests
TestMatrix->>Summary: set XCHR_RESULT_READER
Summary->>LegacyResultReader: parse legacy fixture
Summary->>ModernResultReader: parse modern fixture
LegacyResultReader->>DifferentialTests: return normalized summary
ModernResultReader->>DifferentialTests: return normalized summary
DifferentialTests->>DifferentialTests: compare renders, durations, and attachment bytes
Possibly related PRs
❌ Failed checks (1 warning)
Comment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
Copy link
Copy Markdown
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)Tests/XCTestHTMLReportTests/ModernReaderRuleTests.swift (1)🤖 Prompt for all review comments with AI agentsTests/XCTestHTMLReportTests/KnownLossMasker.swift (1)199-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the temporary bundle copy after the test.
The test copies the fixture and then renders attachments and logs into that copy. Nothing deletes it, so each run leaves a full bundle plus exported payloads in NSTemporaryDirectory().
♻️ Proposed cleanup🤖 Prompt for AI Agentstry FileManager.default.copyItem(at: source, to: copy) + defer { + try? FileManager.default.removeItem(at: copy.deletingLastPathComponent()) + }Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/XCTestHTMLReportTests/ModernReaderRuleTests.swift` around lines 199 - 205, Remove the temporary bundle copy created in ModernReaderRuleTests after the test completes, including when assertions or rendering fail. Use the existing copy URL and ensure cleanup runs after attachments and logs are rendered, without deleting the original fixture.Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader.swift (1)101-113: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Anchor the failureTitlePrefix regexes to the failure-title element.
Both patterns match anywhere in the document. [A-Za-z ]+ at [^\s:]+:\d+:\s* matches ordinary prose in test output, and [\w.+-]+\.swift:\d+:\s* matches any Swift source reference, including one inside a log line or an activity title. A cross-backend divergence in those rows is then masked, and the byte-identity guarantee the allow-list rests on weakens silently.
The durations rule documents its own over-breadth and names testXCTestCaseDurationsAgreeAcrossBackends as the compensating assertion. Apply the same discipline here: scope the pattern to the failure-title element, or add a compensating test that pins failure text on one backend.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/XCTestHTMLReportTests/KnownLossMasker.swift` around lines 101 - 113, Restrict both `failureTitlePrefix` replacement patterns in `KnownLossMasker` to the failure-title element instead of matching anywhere in the HTML document. Preserve removal of both legacy and modern prefixes while ensuring ordinary prose, log lines, and activity titles remain unchanged; alternatively, add a focused compensating test that verifies backend-specific failure text is not masked outside that element.Tests/XCTestHTMLReportTests/ModernPayloadStoreTests.swift (1)275-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Collapse the two startTime checks into one binding.
Line 275 already returns for a nil startTime, so the optional binding on Line 278 always succeeds. One binding states the two drop rules more directly.
♻️ Proposed simplification🤖 Prompt for AI Agents- if child.startTime == nil { + guard let start = child.startTime else { return nil // symbol annotation } - if let start = child.startTime, attachmentTimes.contains(start) { + if attachmentTimes.contains(start) { return nil // attachment shadow } return parsedVerify 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/ModernResultReader.swift` around lines 275 - 280, In the child filtering logic, combine the nil check and attachment-time check into a single optional binding around child.startTime. Return nil when the bound start time is present in attachmentTimes, while preserving the existing behavior of dropping children with nil startTime..github/workflows/test.yml (1)92-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Add a concurrent export assertion to cover the race the fix targets.
The test exports twice in sequence. The reported fault in #449 came from concurrent exports of one destination. Run and TestSummary build attachments on an OperationQueue, so the concurrent path is the one that regressed. Assert it directly.
♻️ Proposed additional assertion🤖 Prompt for AI AgentsXCTAssertTrue( collector.faults.isEmpty, "Re-exporting an already-exported payload is success, not degradation" ) + + // The reported fault came from concurrent exports of one destination. + let concurrent = FaultCollector() + let (parallelStore, parallelBundle) = try store( + for: "TestResults", collector: concurrent + ) + let parallelUUID = try firstAttachmentUUID(in: parallelBundle) + DispatchQueue.concurrentPerform(iterations: 16) { _ in + XCTAssertNotNil( + parallelStore.exportPayload( + reference: parallelUUID, fileName: "0~parallel=.html" + ) + ) + } + XCTAssertTrue( + concurrent.faults.isEmpty, + "Concurrent exports of one content-addressed destination must not fault" + )Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/XCTestHTMLReportTests/ModernPayloadStoreTests.swift` around lines 92 - 115, Extend testExportPayloadIsIdempotentForTheSameDestination to perform two exports of the same reference and fileName concurrently, using the test’s existing concurrency utilities or an appropriate synchronized result collection. Assert both concurrent calls return the same destination, the destination remains valid, and collector.faults stays empty, while preserving the existing sequential idempotency assertions.17-26: 🚀 Performance & Scalability | 🔵 Trivial
Consider warming the fixture cache before the matrix fans out.
Both legs compute the same fixture cache key. On a cold cache both legs boot a simulator and run prepareTestResults.sh, and both then save under one key. The duplicate generation dominates job wall time, as the comment at Line 37 records for the single-leg case. A short prerequisite job that populates the cache would let both legs restore instead of regenerate.
🤖 Prompt for AI Agents
[operational]Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 17 - 26, Update the workflow matrix setup to add a prerequisite cache-warming job that computes the shared fixture cache key, restores it, and runs the fixture-generation flow only on a cache miss before the matrix job starts. Make both matrix legs depend on this warm-up job so they restore the populated cache instead of independently booting simulators and running prepareTestResults.sh; preserve the existing result_reader values and XCHR_RESULT_READER configuration.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@docs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md`: - Around line 451-468: The legacy export path in ResultFile.exportPayload must preserve an existing content-addressed destination: return the destination when it already exists and remove the pre-export deletion so existing payload files are never removed or rewritten. Apply this behavior to the legacy backend, verify both backends satisfy the documented idempotent contract, then update the migration documentation. In `@Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernPayloadStore.swift`: - Around line 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. In `@Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader`+FailureRows.swift: - Around line 102-116: Update removeClaimed so a matched expected-failure activity removes only its row while preserving and returning any attachments or recursively processed subActivities it contains; continue returning nil only when the matched activity has no content to retain, and leave unmatched activity handling unchanged. In `@Tests/XCTestHTMLReportTests/DifferentialTests.swift`: - Around line 304-332: Update the differential test around the payloads helper and legacy/modern summaries to fail when either summary reports export faults, and require every attachment with a payload reference to resolve as .data rather than silently skipping .none content. Preserve the existing filename-count and byte comparisons after these validations. - Around line 159-165: Update testEveryAllowListRuleIsImplemented to assert allowList().knownLosses.count equals 4, then validate that the extracted rule names contain no duplicates before comparing them with KnownLossMasker.implementedRules. Preserve the existing equality assertion for the complete rule set. - Around line 110-150: Replace the aggregate statuses(_:) comparison with per-run identifier-to-status comparisons after the run-count assertion, pairing corresponding legacy and modern runs so duplicate test identifiers from different destinations remain distinct. Preserve the existing status assertion message and add a multi-destination fixture that exercises this behavior, accounting for ModernResultReader producing one ParsedRun per reported device. --- Nitpick comments: In @.github/workflows/test.yml: - Around line 17-26: Update the workflow matrix setup to add a prerequisite cache-warming job that computes the shared fixture cache key, restores it, and runs the fixture-generation flow only on a cache miss before the matrix job starts. Make both matrix legs depend on this warm-up job so they restore the populated cache instead of independently booting simulators and running prepareTestResults.sh; preserve the existing result_reader values and XCHR_RESULT_READER configuration. In `@Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader.swift`: - Around line 275-280: In the child filtering logic, combine the nil check and attachment-time check into a single optional binding around child.startTime. Return nil when the bound start time is present in attachmentTimes, while preserving the existing behavior of dropping children with nil startTime. In `@Tests/XCTestHTMLReportTests/KnownLossMasker.swift`: - Around line 101-113: Restrict both `failureTitlePrefix` replacement patterns in `KnownLossMasker` to the failure-title element instead of matching anywhere in the HTML document. Preserve removal of both legacy and modern prefixes while ensuring ordinary prose, log lines, and activity titles remain unchanged; alternatively, add a focused compensating test that verifies backend-specific failure text is not masked outside that element. In `@Tests/XCTestHTMLReportTests/ModernPayloadStoreTests.swift`: - Around line 92-115: Extend testExportPayloadIsIdempotentForTheSameDestination to perform two exports of the same reference and fileName concurrently, using the test’s existing concurrency utilities or an appropriate synchronized result collection. Assert both concurrent calls return the same destination, the destination remains valid, and collector.faults stays empty, while preserving the existing sequential idempotency assertions. In `@Tests/XCTestHTMLReportTests/ModernReaderRuleTests.swift`: - Around line 199-205: Remove the temporary bundle copy created in ModernReaderRuleTests after the test completes, including when assertions or rendering fail. Use the existing copy URL and ensure cleanup runs after attachments and logs are rendered, without deleting the original fixture.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db4c8ed3-4083-488f-bf33-27ccd918eb85
📥 CommitsReviewing files that changed from the base of the PR and between c461732 and f1dbac0.
📒 Files selected for processing (24)- .github/workflows/test.yml
- Package.swift
- Sources/XCTestHTMLReport/XCTestHtmlReport.swift
- Sources/XCTestHTMLReportCore/Classes/Models/Run.swift
- Sources/XCTestHTMLReportCore/Classes/Models/Summary.swift
- Sources/XCTestHTMLReportCore/Classes/ResultReading/Legacy/LegacyResultReader.swift
- Sources/XCTestHTMLReportCore/Classes/ResultReading/Legacy/ResultFile.swift
- Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernPayloadStore.swift
- Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader+FailureRows.swift
- Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader.swift
- Sources/XCTestHTMLReportCore/Classes/ResultReading/ParsedResult.swift
- Sources/XCTestHTMLReportCore/Classes/ResultReading/ResultBackend.swift
- Sources/XCTestHTMLReportCore/Classes/ResultReading/ResultReader.swift
- Tests/XCTestHTMLReportTests/DifferentialTests.swift
- Tests/XCTestHTMLReportTests/FaultReportingTests.swift
- Tests/XCTestHTMLReportTests/KnownLossMasker.swift
- Tests/XCTestHTMLReportTests/ModernPayloadStoreTests.swift
- Tests/XCTestHTMLReportTests/ModernReaderRuleTests.swift
- Tests/XCTestHTMLReportTests/ModernResultReaderTests.swift
- Tests/XCTestHTMLReportTests/PortRuleTests.swift
- Tests/XCTestHTMLReportTests/Resources/differential-allowlist.json
- Tests/XCTestHTMLReportTests/ResultBackendTests.swift
- docs/superpowers/plans/2026-08-10-xcresulttool-legacy-migration.md
- docs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md
Sorry, something went wrong.
Comment on lines
+451
to
+468
| **Content-addressed attachment exports (fixes #449).** Xcode 26.2 gives every | ||
| auto screen recording in a session one shared display name | ||
| (`Screen Recording <timestamp>.mp4` — in the activities document *and* the | ||
| manifest's `suggestedHumanReadableName`), and the legacy pretty filename | ||
| embeds a legacy-only uuid the modern format cannot see. Naming exports after | ||
| either backend's pretty name therefore broke both ways at once: distinct | ||
| payloads collapsed onto one path (every video row played the same recording), | ||
| concurrent copies raced on it and intermittently recorded a spurious | ||
| `.payloadExportFailed` — root-caused as #449 with a live `EEXIST` trace whose | ||
| `fileExists` recovery check lost to a third thread's `removeItem`. The one | ||
| attachment identifier both formats share is the content-addressed payload id | ||
| (legacy `payloadRef.id` == modern `payloadId`, the same CAS id), so both | ||
| backends name every exported payload | ||
| `ParsedAttachment.exportFileName(payloadId:filenameExtension:)`. Names agree | ||
| by construction, the export is idempotent (a file already at the destination | ||
| *is* the payload — never removed, never rewritten), and byte-identical | ||
| payloads deduplicate to one file. 4.0 release note: on-disk attachment names | ||
| change for every user; display names in the report do not. |
Copy link
Copy Markdown
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make legacy payload exports preserve existing destinations.
This section states that both backends never remove or rewrite an existing payload file. ResultFile.exportPayload still removes its destination before moving the legacy export. A concurrent report process can observe that deletion and reintroduce the export race that this rule documents as fixed.
Return the existing destination for a content-addressed filename. Do not remove it before export. Update the documentation only after both backends meet this contract.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md` around lines 451 - 468, The legacy export path in ResultFile.exportPayload must preserve an existing content-addressed destination: return the destination when it already exists and remove the pre-export deletion so existing payload files are never removed or rewritten. Apply this behavior to the legacy backend, verify both backends satisfy the documented idempotent contract, then update the migration documentation.
Sorry, something went wrong.
Comment on lines
+57
to
70
| // 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 { |
Copy link
Copy Markdown
There was a problem hiding this comment.
🗄️ 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)
+ }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.
Sorry, something went wrong.
Comment thread
Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader+FailureRows.swift
Comment on lines
+102
to
+116
| func removeClaimed(_ activities: [ParsedActivity]) -> [ParsedActivity] { | ||
| activities.compactMap { activity in | ||
| if let index = unclaimed.firstIndex(of: activity.title) { | ||
| unclaimed.remove(at: index) | ||
| return nil | ||
| } | ||
| return ParsedActivity( | ||
| title: activity.title, | ||
| isFailure: activity.isFailure, | ||
| start: activity.start, | ||
| attachments: activity.attachments, | ||
| subActivities: removeClaimed(activity.subActivities) | ||
| ) | ||
| } | ||
| } |
Copy link
Copy Markdown
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Removal of a matched expected-failure row also drops its attachments and children.
removeClaimed returns nil for the matched activity, so its attachments and subActivities disappear with it. The documented intent is to remove the expected-failure row, not user content that happens to hang below it. Keep the content when the matched row carries any.
♻️ Proposed guard activities.compactMap { activity in
- if let index = unclaimed.firstIndex(of: activity.title) {
+ if let index = unclaimed.firstIndex(of: activity.title),
+ activity.attachments.isEmpty, activity.subActivities.isEmpty
+ {
unclaimed.remove(at: index)
return nil
}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/ModernResultReader`+FailureRows.swift around lines 102 - 116, Update removeClaimed so a matched expected-failure activity removes only its row while preserving and returning any attachments or recursively processed subActivities it contains; continue returning nil only when the matched activity has no content to retain, and leave unmatched activity handling unchanged.
Sorry, something went wrong.
Comment thread
Tests/XCTestHTMLReportTests/DifferentialTests.swift
Comment on lines
+110
to
+150
| func statuses(_ summary: Summary) -> [String: String] { | ||
| Dictionary( | ||
| summary.runs.flatMap(\.allTests) | ||
| .map { ($0.identifier, $0.status.rawValue) }, | ||
| uniquingKeysWith: { first, _ in first } | ||
| ) | ||
| } | ||
| XCTAssertEqual( | ||
| statuses(legacy), statuses(modern), | ||
| "\(fixture): identifier→status differs between backends" | ||
| ) | ||
|
|
||
| // Assert before zipping: `zip` truncates to the shorter sequence, | ||
| // so a backend that produced fewer runs would compare equal on the | ||
| // ones it did produce and pass. | ||
| XCTAssertEqual( | ||
| legacy.runs.count, modern.runs.count, | ||
| "\(fixture): backends disagree on the number of runs" | ||
| ) | ||
|
|
||
| for (legacyRun, modernRun) in zip(legacy.runs, modern.runs) { | ||
| XCTAssertEqual( | ||
| legacyRun.numberOfTests, modernRun.numberOfTests, "\(fixture): total" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyRun.numberOfPassedTests, modernRun.numberOfPassedTests, | ||
| "\(fixture): passed" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyRun.numberOfFailedTests, modernRun.numberOfFailedTests, | ||
| "\(fixture): failed" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyRun.numberOfSkippedTests, modernRun.numberOfSkippedTests, | ||
| "\(fixture): skipped" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyRun.numberOfMixedTests, modernRun.numberOfMixedTests, | ||
| "\(fixture): mixed" | ||
| ) | ||
| } |
Copy link
Copy Markdown
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Compare statuses within each run.
statuses(_:) keys only on Test.identifier. The same test identifier occurs on every destination. uniquingKeysWith: { first, _ in first } discards later-device statuses.
Compare identifier-to-status mappings per paired run after the run-count assertion, or include a stable destination key. Add a multi-destination fixture so this contract cannot regress.
Based on learnings: ModernResultReader creates one ParsedRun per reported device, while current fixtures boot only one simulator.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/XCTestHTMLReportTests/DifferentialTests.swift` around lines 110 - 150, Replace the aggregate statuses(_:) comparison with per-run identifier-to-status comparisons after the run-count assertion, pairing corresponding legacy and modern runs so duplicate test identifiers from different destinations remain distinct. Preserve the existing status assertion message and add a multi-destination fixture that exercises this behavior, accounting for ModernResultReader producing one ParsedRun per reported device.
Source: Learnings
Sorry, something went wrong.
Comment thread
Tests/XCTestHTMLReportTests/DifferentialTests.swift
Comment on lines
+159
to
+165
| func testEveryAllowListRuleIsImplemented() throws { | ||
| let declared = try Set(allowList().knownLosses.map(\.rule)) | ||
| XCTAssertEqual( | ||
| declared, KnownLossMasker.implementedRules, | ||
| "The allow-list and KnownLossMasker.implementedRules disagree. " | ||
| + "Every declared rule needs an implementation and vice versa." | ||
| ) |
Copy link
Copy Markdown
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the required allow-list size.
This test verifies only that declared rules equal implemented rules. A fifth declared rule and masker implementation would pass, despite the requirement for exactly four allow-list entries.
Assert that knownLosses.count is 4, and reject duplicate rule names before comparing the rule set.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/XCTestHTMLReportTests/DifferentialTests.swift` around lines 159 - 165, Update testEveryAllowListRuleIsImplemented to assert allowList().knownLosses.count equals 4, then validate that the extracted rule names contain no duplicates before comparing them with KnownLossMasker.implementedRules. Preserve the existing equality assertion for the complete rule set.
Sorry, something went wrong.
Comment thread
Tests/XCTestHTMLReportTests/DifferentialTests.swift
Comment on lines
+304
to
+332
| func payloads(_ summary: Summary) -> [String: [Data]] { | ||
| var byName: [String: [Data]] = [:] | ||
| for attachment in summary.allAttachments { | ||
| guard case let .data(data) = attachment.content else { | ||
| continue | ||
| } | ||
| byName[attachment.filename, default: []].append(data) | ||
| } | ||
| return byName.mapValues { $0.sorted { $0.count < $1.count } } | ||
| } | ||
|
|
||
| for fixture in Self.fixtures { | ||
| // Rendered inline so the bytes are in hand rather than on disk. | ||
| let legacy = try summaryInline(fixture, .legacy) | ||
| let modern = try summaryInline(fixture, .modern) | ||
| let legacyPayloads = payloads(legacy) | ||
| XCTAssertFalse( | ||
| legacyPayloads.isEmpty, | ||
| "\(fixture): no attachment bytes to compare — the assertion " | ||
| + "below would pass vacuously" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyPayloads.mapValues(\.count), | ||
| payloads(modern).mapValues(\.count), | ||
| "\(fixture): attachment counts differ between backends" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyPayloads, payloads(modern), | ||
| "\(fixture): attachment bytes differ between backends" |
Copy link
Copy Markdown
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail when an attachment payload does not resolve.
The guard case let .data(data) branch skips .none content. If one attachment fails to export on both backends, this comparison can still pass because neither its filename nor its missing bytes enter either map.
Assert that both summaries have no export faults. Also require every attachment with a payload reference to resolve to data before comparing filename counts and byte collections.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/XCTestHTMLReportTests/DifferentialTests.swift` around lines 304 - 332, Update the differential test around the payloads helper and legacy/modern summaries to fail when either summary reports export faults, and require every attachment with a payload reference to resolve as .data rather than silently skipping .none content. Preserve the existing filename-count and byte comparisons after these validations.
Sorry, something went wrong.
tylervick
added a commit
that referenced
this pull request
Aug 13, 2026
…elease notes (#391, Tasks 14–15) (#451) * feat!: emit our own schema from --json instead of the legacy object graph BREAKING: --json previously dumped xcresulttool's legacy object graph verbatim. That graph is Apple's internal shape and disappears with the legacy commands, so --json now emits a documented schema, identical on both backends. docs/json-schema.md is the contract, written before the encoder: field names and nesting with a complete worked example, enum spellings, one uniform null rule, duration and timestamp formats, ordering guarantees, and a semver schemaVersion policy. The encoder (JsonReport.swift) is an explicit layer rather than a synthesized Encodable on the internal model, so renaming a Parsed* property breaks compilation instead of silently renaming public output. ResultFile.exportJson() — the last exportRecursiveJson() call, kept since Task 5a — is deleted; XCResultKit is confined to ResultReading/Legacy/. JsonReportTests holds the output to the contract across both backends: recursive schema identity, values deeply equal outside the two permitted difference classes (JsonClassMask masks exactly the declared losses, with non-vacuity stats), and arguments compared per class 2 — legacy asserted == [] explicitly, modern asserted non-empty for the parameterized fixture. The value differential surfaced one reader-parity gap the HTML differential cannot see: under a retry-enabled plan, legacy stamps every summary "iteration 1" while modern reports repetition info only for real repetitions, and nothing renders a lone iteration number. The legacy reader now strips it (strippingLoneIterationNumber) — a single execution carries no repetition information on either backend. Recorded as a Task 14 execution rule in the spec. Refs #391 (Task 14). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: fold in the carried review findings from #447 and #450 - Pin the no-startTime keep-guard (#450 review): a no-start activity row carrying an attachment, a failure flag, or surviving children is kept by the symbol-annotation drop; only the contentless row is an annotation. Crafted-document test, since no fixture produces the shape. - Suffix the failure-artifact name per matrix leg in test.yml (#450 review): upload-artifact v4 refuses duplicate names, so a run where both legs fail would have lost the second upload. - ModernPayloadStore takes XCResultToolInvoking instead of the concrete client (#447 review), so export-failure faulting is provable in-suite; the new test also pins that a failed one-shot export no longer leaks its temp directory (removed in the catch — deinit never saw it). - An out-of-range repetition index no longer falls back silently to runs.first (#447 review): the iteration renders no activities and records .missingActivities, instead of borrowing repetition 1's rows. Refs #391. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: document --result-reader and the --json schema change README gains the --result-reader section, the --json break with a before/after snippet, and the known backend differences drawn from the allow-list. The 4.0 release notes are drafted at docs/release-notes/4.0.0.md in the 3.0.0 narrative style for the release to source, covering the full Task 15 checklist including the items added by rulings R1/R5/R7 and the #443/#450 reviews. The spec gains a status header (phases 1-5 landed, phase 6 deliberately deferred) and the Task 14 execution rules; the plan's Tasks 14-15 are ticked with implementation amendments recorded. Refs #391 (Task 15). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: state schema identity, not value identity, in the README The --json section claimed the file is identical across readers; the contract's own headline is schema identity, with declared value differences. Point at those and at testCase.arguments, the modern-only capability. Addresses the valid half of CodeRabbit's review on #451. Refs #391. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 14, 2026
This was referenced Aug 14, 2026
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
THE DIFFERENTIAL — the test this migration exists to pass, landed while both backends still exist to compare. Renders every fixture through both readers, asserts structural equality exactly, and holds the HTML diff to the four declared allow-list entries — nothing more, nothing less, enforced in both directions. Plus the forced-modern CI leg so both paths run on every PR (Tasks 12–13 of the migration plan).
Fixes #449. Refs #391. Milestone 4.0.
Differential results (Xcode 26.2 17C52, xcresulttool 24514, fresh fixture generation)
swift test --filter DifferentialTests — all 6 tests pass:
Allow-list anti-rot (necessity probe: a rule "fires" only if omitting it leaves the renders unequal):
All four entries fire; a diff outside the list fails the build; an entry that fires nowhere also fails (testEveryAllowListEntryStillMasksARealDivergence), so entries cannot rot once Apple fills a gap. The list stands at exactly the spec's four entries — the first differential run added none.
#449: reproduced, root-caused, fixed
The rulings (all coordinator-approved; spec's new "Task 12 execution rules" section)
The first real differential run surfaced divergences the spec's tables didn't anticipate. Every resolution follows the settled doctrine — agree by construction over mask; never model what only one backend can fill:
Task 13: forced-modern CI leg
Suite results (local, this generation)
Docs amended in this PR (rules with reasoning, not changelog notes)
New tests
DifferentialTests (6), PortRuleTests (8: interleave totality incl. the real ms-collision ties, argument-execution merge boundaries, export-name derivation), ModernReaderRuleTests (5: crafted-document tip extraction — multi-tip chains and flagged leaves — shadow-join survivor pin, expected-failure removal pin, content-addressed link resolution), store idempotency, env-var override. Existing testNestedRetryFailureIsRetitledInPlace updated to the hoist contract (…RetitledAndHoisted).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests