Skip to content
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

Improve diffing of collections and structured values. #65

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 33 additions & 2 deletions Sources/Testing/Events/Event.Recorder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,37 @@ extension Event.Recorder {
}
}

// MARK: -

extension Difference {
/// Get a description of this instance with the given recorder options.
///
/// - Parameters:
/// - options: Options to use when writing the comments.
///
/// - Returns: A formatted description of `self`.
fileprivate func formattedDescription(options: Set<Event.Recorder.Option>) -> String {
guard options.contains(.useANSIEscapeCodes) else {
return String(describing: self)
}

return String(describing: self)
.split(whereSeparator: \.isNewline)
.map { line in
switch line.first {
case "+":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking about refactoring this code a bit in the future to use SF Symbols when available, but I want to factor _Symbol out into its own file first, and that's not relevant to this PR.

"\(_ansiEscapeCodePrefix)32m\(line)\(_resetANSIEscapeCode)"
case "-":
"\(_ansiEscapeCodePrefix)31m\(line)\(_resetANSIEscapeCode)"
default:
String(line)
}
}.joined(separator: "\n")
}
}

// MARK: -

extension Tag {
/// Get an ANSI escape code that sets the foreground text color to this tag's
/// corresponding color, if applicable.
Expand Down Expand Up @@ -553,9 +584,9 @@ extension Event.Recorder {
}

var difference = ""
if case let .expectationFailed(expectation) = issue.kind, let differenceDescription = expectation.differenceDescription {
if case let .expectationFailed(expectation) = issue.kind, let differenceValue = expectation.difference {
let differenceSymbol = _Symbol.difference.stringValue(options: options)
difference = "\n\(differenceSymbol) \(differenceDescription)"
difference = "\n\(differenceSymbol) \(differenceValue.formattedDescription(options: options))"
}

var issueComments = ""
Expand Down
148 changes: 148 additions & 0 deletions Sources/Testing/Expectations/Difference.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for Swift project authors
//

/// A type that describes the difference between two values as a sequence of
/// insertions, deletions, or unmodified elements.
///
/// To ensure that ``Difference`` can always conform to `Sendable`, the elements
/// in an instance of this type are stored as strings rather than as their
/// original types. They are converted to strings using
/// ``Swift/String/init(describingForTest:)``. Types can implement
/// ``CustomTestStringConvertible`` to customize how they appear in the
/// description of an instance of this type.
@_spi(ExperimentalEventHandling)
public struct Difference: Sendable {
/// A type representing an element of a collection that may have been changed
/// after diffing was applied.
///
/// This type roughly approximates `CollectionDifference.Change`, however it
/// is used to track _all_ elements in the collection, not just those that
/// have changed, allowing for insertion of "marker" elements where removals
/// occurred.
enum Element: Sendable {
/// The element did not change during diffing.
case unchanged(String)

/// The element was inserted.
case inserted(String)

/// The element was removed.
case removed(String)

/// The element replaced a previous value.
///
/// - Parameters:
/// - oldValue: The old value at this position.
/// - with: The new value at this position.
case replaced(_ oldValue: String, with: String)
}

/// The changed elements from the comparison.
var elements = [Element]()

/// Initialize an instance of this type by comparing two collections.
///
/// - Parameters:
/// - lhs: The "old" state of the collection to compare.
/// - rhs: The "new" state of the collection to compare.
init?<T, U>(from lhs: T, to rhs: U) {
let lhsDump = String(describingForTestComparison: lhs)
.split(whereSeparator: \.isNewline)
.map(String.init)
let rhsDump = String(describingForTestComparison: rhs)
.split(whereSeparator: \.isNewline)
.map(String.init)

guard lhsDump.count > 1 || rhsDump.count > 1 else {
return nil
}

// Compute the difference between the two elements. Sort the resulting set
// of changes by their offsets, and ensure that insertions come before
// removals located at the same offset. This helps to ensure that the offset
// values do not drift as we walk the changeset.
let difference = rhsDump.difference(from: lhsDump)

// Walk the initial string and slowly transform it into the final string.
// Add an additional "scratch" string that is used to store a removal marker
// if the last character is removed.
var result = lhsDump.map { [Element.unchanged($0)] } + CollectionOfOne([])
for change in difference.removals.reversed() {
// Remove the character at the specified index, then re-insert it into the
// slot at the next index (with the marker character applied.) The next
// index will then contain whatever character it already contained after
// the character representing this removal.
result.remove(at: change.offset)
result[change.offset].insert(.removed(change.element), at: 0)
}
for change in difference.insertions {
// Insertions can occur verbatim by inserting a new substring at the
// specified offset.
result.insert([.inserted(change.element)], at: change.offset)
}

// Flatten the arrays of arrays of elements into a single array, then merge
// pairs of removals/insertions (i.e. where an element was replaced with
// another element) because it's easier to do after the array of arrays has
// been flatted.
elements = result.lazy
.flatMap { $0 }
.reduce(into: []) { result, element in
if case let .removed(removedValue) = element, let previous = result.last, case let .inserted(insertedValue) = previous {
result[result.index(before: result.endIndex)] = .replaced(removedValue, with: insertedValue)
} else {
result.append(element)
}
}
}
}

// MARK: - Equatable

extension Difference.Element: Equatable {}

// MARK: - Codable

extension Difference: Codable {}
extension Difference.Element: Codable {}

// MARK: - CustomStringConvertible

extension Difference: CustomStringConvertible {
public var description: String {
// Show individual lines of the text with leading + or - characters to
// indicate insertions and removals respectively.
// FIXME: better descriptive output for one-line strings.
let diffCount = elements.lazy
.filter { element in
if case .unchanged = element {
return false
}
return true
}.count.counting("change")

return "\(diffCount):\n" + elements.lazy
.flatMap { element in
switch element {
case let .unchanged(value):
[" \(value)"]
case let .inserted(value):
["+ \(value)"]
case let .removed(value):
["- \(value)"]
case let .replaced(oldValue, with: newValue):
[
"- \(oldValue)",
"+ \(newValue)"
]
}
}.joined(separator: "\n")
}
}
15 changes: 9 additions & 6 deletions Sources/Testing/Expectations/Expectation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,17 @@ public struct Expectation: Sendable {
@_spi(ExperimentalEventHandling)
public var expandedExpressionDescription: String?

/// A description of the difference between the operands in the expression
/// evaluated by this expectation, if the difference could be determined.
/// An opaque value that describes the difference between two values or
/// collections that have been compared.
///
/// If this expectation passed, the value of this property is `nil` because
/// the difference is only computed when necessary to assist with diagnosing
/// test failures.
/// If this expectation did not involve the comparison of two values or
/// collections, the value of this property is `nil`.
///
/// If this expectation passed, the value of this property will be `nil`
/// because the difference is only computed when necessary to assist with
/// diagnosing failures.
@_spi(ExperimentalEventHandling)
public var differenceDescription: String?
public var difference: Difference?

/// Whether the expectation passed or failed.
///
Expand Down
119 changes: 40 additions & 79 deletions Sources/Testing/Expectations/ExpectationChecking+Macro.swift
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,16 @@
/// not we throw that error during macro resolution without affecting any errors
/// thrown by the condition expression passed to it.
///
/// This overload serves as the bottleneck that all other overloads call.
///
/// - Warning: This function is used to implement the `#expect()` and
/// `#require()` macros. Do not call it directly.
public func __checkValue(
func __checkValue(
_ condition: Bool,
sourceCode: SourceCode,
expandedExpressionDescription: @autoclosure () -> String? = nil,
mismatchedErrorDescription: @autoclosure () -> String? = nil,
difference: @autoclosure () -> String? = nil,
difference: @autoclosure () -> Difference? = nil,
comments: @autoclosure () -> [Comment],
isRequired: Bool,
sourceLocation: SourceLocation
Expand All @@ -83,7 +85,7 @@ public func __checkValue(
// only evaluated and included lazily upon failure.
expectation.expandedExpressionDescription = expandedExpressionDescription()
expectation.mismatchedErrorDescription = mismatchedErrorDescription()
expectation.differenceDescription = difference()
expectation.difference = difference()

// Ensure the backtrace is captured here so it has fewer extraneous frames
// from the testing framework which aren't relevant to the user.
Expand All @@ -92,6 +94,33 @@ public func __checkValue(
return .failure(ExpectationFailedError(expectation: expectation))
}

/// Check that an expectation has passed after a condition has been evaluated
/// and throw an error if it failed.
///
/// This overload is the fallback overload used when no other more-precise
/// overload applies.
///
/// - Warning: This function is used to implement the `#expect()` and
/// `#require()` macros. Do not call it directly.
public func __checkValue(
_ condition: Bool,
sourceCode: SourceCode,
expandedExpressionDescription: @autoclosure () -> String? = nil,
comments: @autoclosure () -> [Comment],
isRequired: Bool,
sourceLocation: SourceLocation
) -> Result<Void, any Error> {
__checkValue(
condition,
sourceCode: sourceCode,
expandedExpressionDescription: expandedExpressionDescription(),
difference: nil,
comments: comments(),
isRequired: isRequired,
sourceLocation: sourceLocation
)
}

// MARK: - Binary operators

/// Call a binary operator, passing the left-hand and right-hand arguments.
Expand Down Expand Up @@ -147,10 +176,17 @@ public func __checkBinaryOperation<T, U>(
sourceLocation: SourceLocation
) -> Result<Void, any Error> {
let (condition, rhs) = _callBinaryOperator(lhs, op, rhs)
func difference() -> Difference? {
guard let rhs else {
return nil
}
return Difference(from: lhs, to: rhs)
}
return __checkValue(
condition,
sourceCode: sourceCode,
expandedExpressionDescription: sourceCode.expandWithOperands(lhs, rhs),
difference: difference(),
comments: comments(),
isRequired: isRequired,
sourceLocation: sourceLocation
Expand Down Expand Up @@ -277,81 +313,7 @@ public func __checkInoutFunctionCall<T, /*each*/ U, R>(
)
}

// MARK: - Collection diffing

/// Check that an expectation has passed after a condition has been evaluated
/// and throw an error if it failed.
///
/// This overload is used to implement difference-reporting support when
/// comparing collections.
///
/// - Warning: This function is used to implement the `#expect()` and
/// `#require()` macros. Do not call it directly.
public func __checkBinaryOperation<T>(
_ lhs: T, _ op: (T, () -> T) -> Bool, _ rhs: @autoclosure () -> T,
sourceCode: SourceCode,
comments: @autoclosure () -> [Comment],
isRequired: Bool,
sourceLocation: SourceLocation
) -> Result<Void, any Error> where T: BidirectionalCollection, T.Element: Equatable {
let (condition, rhs) = _callBinaryOperator(lhs, op, rhs)
func difference() -> String? {
guard let rhs else {
return nil
}
let difference = lhs.difference(from: rhs)
let insertions = difference.insertions.map(\.element)
let removals = difference.removals.map(\.element)
switch (!insertions.isEmpty, !removals.isEmpty) {
case (true, true):
return "inserted \(insertions), removed \(removals)"
case (true, false):
return "inserted \(insertions)"
case (false, true):
return "removed \(removals)"
case (false, false):
return ""
}
}

return __checkValue(
condition,
sourceCode: sourceCode,
expandedExpressionDescription: sourceCode.expandWithOperands(lhs, rhs),
difference: difference(),
comments: comments(),
isRequired: isRequired,
sourceLocation: sourceLocation
)
}

/// Check that an expectation has passed after a condition has been evaluated
/// and throw an error if it failed.
///
/// This overload is necessary because `String` satisfies the requirements for
/// the difference-calculating overload above, but the output from that overload
/// may be unexpectedly complex.
///
/// - Warning: This function is used to implement the `#expect()` and
/// `#require()` macros. Do not call it directly.
public func __checkBinaryOperation(
_ lhs: String, _ op: (String, () -> String) -> Bool, _ rhs: @autoclosure () -> String,
sourceCode: SourceCode,
comments: @autoclosure () -> [Comment],
isRequired: Bool,
sourceLocation: SourceLocation
) -> Result<Void, any Error> {
let (condition, rhs) = _callBinaryOperator(lhs, op, rhs)
return __checkValue(
condition,
sourceCode: sourceCode,
expandedExpressionDescription: sourceCode.expandWithOperands(lhs, rhs),
difference: nil,
comments: comments(),
isRequired: isRequired,
sourceLocation: sourceLocation
)
}
// MARK: - Type checks

/// Check that an expectation has passed after a condition has been evaluated
/// and throw an error if it failed.
Expand All @@ -372,7 +334,6 @@ public func __checkCast<V, T>(
value is T,
sourceCode: sourceCode,
expandedExpressionDescription: sourceCode.expandWithOperands(value, type(of: value as Any)),
difference: nil,
comments: comments(),
isRequired: isRequired,
sourceLocation: sourceLocation
Expand Down