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

Limit the amount of value reflection data expectation checking collects by default #915

Merged
merged 3 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/Testing/Parameterization/Test.Case.swift
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ extension Test.Case.Argument {
/// - argument: The original test case argument to snapshot.
public init(snapshotting argument: Test.Case.Argument) {
id = argument.id
value = Expression.Value(reflecting: argument.value)
value = Expression.Value(reflecting: argument.value) ?? .init(describing: argument.value)
parameter = argument.parameter
}
}
Expand Down
38 changes: 38 additions & 0 deletions Sources/Testing/Running/Configuration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,44 @@ public struct Configuration: Sendable {

/// The test case filter to which test cases should be filtered when run.
public var testCaseFilter: TestCaseFilter = { _, _ in true }

// MARK: - Expectation checking

/// Whether or not reflection of values in expressions checked by expectations
/// is enabled.
///
/// When the value of this property is `false`, value reflection is disabled
/// and values will only be represented using `String(describing:)` instead of
/// using `Mirror` to recursively reflect their substructure.
public var isValueReflectionEnabled: Bool = true

/// The maximum number of elements that can included in a single child
/// collection when reflecting a value checked by an expectation.
///
/// When ``Expression/Value/init(reflecting:)`` is reflecting a value and it
/// encounters a child value which is a collection, it consults the value of
/// this property and only includes the children of that collection up to this
/// maximum count. After this maximum is reached, all subsequent elements are
/// omitted and a single placeholder child is added indicating the number of
/// elements which have been truncated.
public var maximumValueReflectionCollectionCount: Int = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public var maximumValueReflectionCollectionCount: Int = 10
public var maximumValueReflectionCollectionCount = 10

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 like having the types of properties with public or greater access level state their type explicitly, to avoid the possibility of the inferred type changing without our knowledge and causing a API breakage. I see now that we have not done this with some trivial properties in Configuration, but now that I've moved most of these into a nested struct I've used this style within that new struct at least. I'd like to apply this coding style more broadly in a subsequent PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

We pretty much never do this for variables to which we assign values. We only specify types on variables that will be assigned values later. Consistency would be good (either way.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already merged, but I did strip off the explicit types from these properties for now, for consistency!


/// The maximum depth of children that can be included in the reflection of a
/// checked expectation value.
///
/// When ``Expression/Value/init(reflecting:)`` is reflecting a value, it
/// recursively reflects that value's children. Before doing so, it consults
/// the value of this property to determine the maximum depth of the children
/// to include. After this maximum depth is reached, all children at deeper
/// levels are omitted and the ``Expression/Value/isTruncated`` property is
/// set to `true` to reflect that the reflection is incomplete.
///
/// - Note: `Optional` values contribute twice towards this maximum, since
/// their mirror represents the wrapped value as a child of the optional.
/// Since optionals are common, the default value of this property is
/// somewhat larger than it otherwise would be in an attempt to make the
/// defaults useful for real-world tests.
public var maximumValueReflectionChildDepth: Int = 10
}

// MARK: - Deprecated
Expand Down
21 changes: 21 additions & 0 deletions Sources/Testing/Running/Runner.RuntimeState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,27 @@ extension Configuration {
/// - Returns: Whatever is returned by `body`.
///
/// - Throws: Whatever is thrown by `body`.
static func withCurrent<R>(_ configuration: Self, perform body: () throws -> R) rethrows -> R {
let id = configuration._addToAll()
defer {
configuration._removeFromAll(identifiedBy: id)
}

var runtimeState = Runner.RuntimeState.current ?? .init()
runtimeState.configuration = configuration
return try Runner.RuntimeState.$current.withValue(runtimeState, operation: body)
}

/// Call an asynchronous function while the value of ``Configuration/current``
/// is set.
///
/// - Parameters:
/// - configuration: The new value to set for ``Configuration/current``.
/// - body: A function to call.
///
/// - Returns: Whatever is returned by `body`.
///
/// - Throws: Whatever is thrown by `body`.
static func withCurrent<R>(_ configuration: Self, perform body: () async throws -> R) async rethrows -> R {
let id = configuration._addToAll()
defer {
Expand Down
95 changes: 74 additions & 21 deletions Sources/Testing/SourceAttribution/Expression.swift
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,16 @@ public struct __Expression: Sendable {
/// property is `nil`.
public var label: String?

/// Whether or not the values of certain properties of this instance have
/// been truncated for brevity.
///
/// If the value of this property is `true`, this instance does not
/// represent its original value completely because doing so would exceed
/// the maximum allowed data collection settings of the ``Configuration`` in
/// effect. When this occurs, the value ``children`` is not guaranteed to be
/// accurate or complete.
public var isTruncated: Bool = false

/// Whether or not this value represents a collection of values.
public var isCollection: Bool

Expand All @@ -167,14 +177,32 @@ public struct __Expression: Sendable {
/// the value it represents contains substructural values.
public var children: [Self]?

/// Initialize an instance of this type describing the specified subject.
///
/// - Parameters:
/// - subject: The subject this instance should describe.
init(describing subject: Any) {
description = String(describingForTest: subject)
debugDescription = String(reflecting: subject)
typeInfo = TypeInfo(describingTypeOf: subject)

let mirror = Mirror(reflecting: subject)
isCollection = mirror.displayStyle?.isCollection ?? false
}

/// Initialize an instance of this type describing the specified subject and
/// its children (if any).
///
/// - Parameters:
/// - subject: The subject this instance should describe.
init(reflecting subject: Any) {
/// - subject: The subject this instance should reflect.
init?(reflecting subject: Any) {
let configuration = Configuration.current ?? .init()
guard configuration.isValueReflectionEnabled else {
return nil
}

var seenObjects: [ObjectIdentifier: AnyObject] = [:]
self.init(_reflecting: subject, label: nil, seenObjects: &seenObjects)
self.init(_reflecting: subject, label: nil, seenObjects: &seenObjects, depth: 0)
}

/// Initialize an instance of this type describing the specified subject and
Expand All @@ -189,11 +217,27 @@ public struct __Expression: Sendable {
/// this initializer recursively, keyed by their object identifiers.
/// This is used to halt further recursion if a previously-seen object
/// is encountered again.
/// - depth: The depth of this recursive call.
private init(
_reflecting subject: Any,
label: String?,
seenObjects: inout [ObjectIdentifier: AnyObject]
seenObjects: inout [ObjectIdentifier: AnyObject],
depth: Int
) {
let configuration = Configuration.current ?? .init()

// Stop recursing if we've reached the maximum allowed depth for
// reflection. Instead, return a node describing this value instead and
// set `isTruncated` to `true`.
if depth >= configuration.maximumValueReflectionChildDepth {
self = Self(describing: subject)
isTruncated = true
return
}

self.init(describing: subject)
self.label = label

let mirror = Mirror(reflecting: subject)

// If the subject being reflected is an instance of a reference type (e.g.
Expand Down Expand Up @@ -236,24 +280,19 @@ public struct __Expression: Sendable {
}
}

description = String(describingForTest: subject)
debugDescription = String(reflecting: subject)
typeInfo = TypeInfo(describingTypeOf: subject)
self.label = label

isCollection = switch mirror.displayStyle {
case .some(.collection),
.some(.dictionary),
.some(.set):
true
default:
false
}

if shouldIncludeChildren && (!mirror.children.isEmpty || isCollection) {
self.children = mirror.children.map { child in
Self(_reflecting: child.value, label: child.label, seenObjects: &seenObjects)
var children: [Self] = []
for (index, child) in mirror.children.enumerated() {
if isCollection && index >= configuration.maximumValueReflectionCollectionCount {
isTruncated = true
let message = "(\(mirror.children.count - index) elements (out of \(mirror.children.count) total) omitted for brevity.)"
children.append(Self(describing: message))
break
}

children.append(Self(_reflecting: child.value, label: child.label, seenObjects: &seenObjects, depth: depth + 1))
}
self.children = children
}
}
}
Expand All @@ -274,7 +313,7 @@ public struct __Expression: Sendable {
/// value captured for future use.
func capturingRuntimeValue(_ value: (some Any)?) -> Self {
var result = self
result.runtimeValue = value.map { Value(reflecting: $0) }
result.runtimeValue = value.flatMap(Value.init(reflecting:))
if case let .negation(subexpression, isParenthetical) = kind, let value = value as? Bool {
result.kind = .negation(subexpression.capturingRuntimeValue(!value), isParenthetical: isParenthetical)
}
Expand Down Expand Up @@ -547,3 +586,17 @@ extension __Expression.Value: CustomStringConvertible, CustomDebugStringConverti
/// ```
@_spi(ForToolsIntegrationOnly)
public typealias Expression = __Expression

extension Mirror.DisplayStyle {
/// Whether or not this display style represents a collection of values.
fileprivate var isCollection: Bool {
switch self {
case .collection,
.dictionary,
.set:
true
default:
false
}
}
}
90 changes: 82 additions & 8 deletions Tests/TestingTests/Expression.ValueTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ struct Expression_ValueTests {

let foo = Foo()

let value = Expression.Value(reflecting: foo)
let value = try #require(Expression.Value(reflecting: foo))
let children = try #require(value.children)
try #require(children.count == 1)

Expand Down Expand Up @@ -49,7 +49,7 @@ struct Expression_ValueTests {
x.one = y
x.two = y

let value = Expression.Value(reflecting: x)
let value = try #require(Expression.Value(reflecting: x))
let children = try #require(value.children)
try #require(children.count == 3)

Expand Down Expand Up @@ -78,7 +78,7 @@ struct Expression_ValueTests {
x.two = y
y.two = x

let value = Expression.Value(reflecting: x)
let value = try #require(Expression.Value(reflecting: x))
let children = try #require(value.children)
try #require(children.count == 3)

Expand Down Expand Up @@ -116,7 +116,7 @@ struct Expression_ValueTests {
let recursiveItem = RecursiveItem()
recursiveItem.anotherItem = recursiveItem

let value = Expression.Value(reflecting: recursiveItem)
let value = try #require(Expression.Value(reflecting: recursiveItem))
let children = try #require(value.children)
try #require(children.count == 2)

Expand All @@ -142,7 +142,7 @@ struct Expression_ValueTests {
one.two = two
two.one = one

let value = Expression.Value(reflecting: one)
let value = try #require(Expression.Value(reflecting: one))
let children = try #require(value.children)
try #require(children.count == 1)

Expand All @@ -168,7 +168,7 @@ struct Expression_ValueTests {

@Test("Value reflecting an object with two back-references to itself",
.bug("https://github.com/swiftlang/swift-testing/issues/785#issuecomment-2440222995"))
func multipleSelfReferences() {
func multipleSelfReferences() throws {
class A {
weak var one: A?
weak var two: A?
Expand All @@ -178,7 +178,7 @@ struct Expression_ValueTests {
a.one = a
a.two = a

let value = Expression.Value(reflecting: a)
let value = try #require(Expression.Value(reflecting: a))
#expect(value.children?.count == 2)
}

Expand Down Expand Up @@ -208,8 +208,82 @@ struct Expression_ValueTests {
b.c = c
c.a = a

let value = Expression.Value(reflecting: a)
let value = try #require(Expression.Value(reflecting: a))
#expect(value.children?.count == 3)
}

@Test("Value reflection can be disabled via Configuration")
func valueReflectionDisabled() {
var configuration = Configuration.current ?? .init()
configuration.isValueReflectionEnabled = false
Configuration.withCurrent(configuration) {
#expect(Expression.Value(reflecting: "hello") == nil)
}
}

@Test("Value reflection truncates large values")
func reflectionOfLargeValues() throws {
struct Large {
var foo: Int?
var bar: [Int]
}

var configuration = Configuration.current ?? .init()
configuration.maximumValueReflectionCollectionCount = 2
configuration.maximumValueReflectionChildDepth = 2
try Configuration.withCurrent(configuration) {
let large = Large(foo: 123, bar: [4, 5, 6, 7])
let value = try #require(Expression.Value(reflecting: large))

#expect(!value.isTruncated)
do {
let fooValue = try #require(value.children?.first)
#expect(!fooValue.isTruncated)
let fooChildren = try #require(fooValue.children)
try #require(fooChildren.count == 1)
let fooChild = try #require(fooChildren.first)
#expect(fooChild.isTruncated)
#expect(fooChild.children == nil)
}
do {
let barValue = try #require(value.children?.last)
#expect(barValue.isTruncated)
#expect(barValue.children?.count == 3)
let lastBarChild = try #require(barValue.children?.last)
#expect(String(describing: lastBarChild) == "(2 elements (out of 4 total) omitted for brevity.)")
}
}
}

@Test("Value reflection max collection count only applies to collections")
func reflectionMaximumCollectionCount() throws {
struct X {
var a = 1
var b = 2
var c = 3
var d = 4
}

var configuration = Configuration.current ?? .init()
configuration.maximumValueReflectionCollectionCount = 2
try Configuration.withCurrent(configuration) {
let x = X()
let value = try #require(Expression.Value(reflecting: x))
#expect(!value.isTruncated)
#expect(value.children?.count == 4)
}
}

@Test("Value describing a simple struct")
func describeSimpleStruct() {
struct Foo {
var x: Int = 123
}

let foo = Foo()
let value = Expression.Value(describing: foo)
#expect(String(describing: value) == "Foo(x: 123)")
#expect(value.children == nil)
}

}