Skip to content

Lower some symbols to SPI. #548

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 17, 2024
Merged

Lower some symbols to SPI. #548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b941672
Lower some symbols to SPI.
grynspan Jul 17, 2024
5f2f0de
Remove now-SPI symbol from docs
grynspan Jul 17, 2024
b1fbb57
Test.Case conforms to Identifiable, but its conformance is marked SPI…
grynspan Jul 17, 2024
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
1 change: 1 addition & 0 deletions Sources/Testing/Issues/Issue.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ extension Issue {
public var sourceContext: SourceContext

/// Whether or not this issue is known to occur.
@_spi(ForToolsIntegrationOnly)
public var isKnown = false

/// Initialize an issue instance with the specified details.
Expand Down
3 changes: 2 additions & 1 deletion Sources/Testing/Parameterization/Test.Case.ID.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
// See https://swift.org/CONTRIBUTORS.txt for Swift project authors
//

@_spi(ForToolsIntegrationOnly)
Copy link
Contributor

@stmontgomery stmontgomery Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm worried about this change -- making the Identifiable conformance public and also making its ID type public. Test.Case.ID is not yet fully designed and, in some scenarios will not actually uniquely identify a test case. When its argumentIDs property is nil (which is the case if any argument is non-Codable or equivalent), it will be considered equal to all other instances representing non-Codable arguments. This could be confusing to users or lead to bugs.

This is a known limitation of this ID type currently which we need to come back and fix, but the current tool adopters already use it properly with that consideration in mind. But if we make this change it will publish this ID type as API and I don't think it is mature or robust enough yet.

The existing solution was to wrap this entire extension in @_spi. Can you elaborate on why that is problematic? A runtime as? Identifiable cast may succeed, but since the concrete ID type is not public, I did not think this would be a problem.

Copy link
Contributor Author

@grynspan grynspan Jul 17, 2024

Choose a reason for hiding this comment

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

Protocol conformances are always as visible as the type/protocol in question. See here: https://developer.apple.com/documentation/testing/test/case#relationships

If we don't want the conformance to be public, we'll have to remove it outright.

Copy link
Contributor Author

@grynspan grynspan Jul 17, 2024

Choose a reason for hiding this comment

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

Behold:

func idForTestCase(_ testCase: Test.Case) -> some Hashable {
  func thunk(_ x: some Identifiable) -> some Hashable {
    x.id
  }
  return thunk(testCase)
}

Contrived, but the point is the IDs are publicly accessible already.

Copy link
Contributor Author

@grynspan grynspan Jul 17, 2024

Choose a reason for hiding this comment

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

Actually, due to opaque types, this is valid even when everything's SPI and you haven't imported it:

let x = Test.Case.current!.id
let y = Test.Case.current!.id
#expect(x == y)

So yeah, we need to pick a side of the fence to be on here.

Copy link
Contributor

@stmontgomery stmontgomery Jul 17, 2024

Choose a reason for hiding this comment

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

Yeah, let's just remove the Identifiable conformance

Copy link
Contributor Author

@grynspan grynspan Jul 17, 2024

Choose a reason for hiding this comment

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

@briancroom Thoughts?

extension Test.Case: Identifiable {
/// The ID of a test case.
///
Expand All @@ -21,8 +20,10 @@ extension Test.Case: Identifiable {
///
/// The value of this property is `nil` if _any_ of the associated test
/// case's arguments has a `nil` ID.
@_spi(ForToolsIntegrationOnly)
public var argumentIDs: [Argument.ID]?

@_spi(ForToolsIntegrationOnly)
public init(argumentIDs: [Argument.ID]?) {
self.argumentIDs = argumentIDs
}
Expand Down
3 changes: 3 additions & 0 deletions Sources/Testing/Parameterization/Test.Case.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ extension Test {
public struct Case: Sendable {
/// A type representing an argument passed to a parameter of a parameterized
/// test function.
@_spi(Experimental) @_spi(ForToolsIntegrationOnly)
public struct Argument: Sendable {
/// A type representing the stable, unique identifier of a parameterized
/// test argument.
Expand Down Expand Up @@ -71,6 +72,7 @@ extension Test {
/// Non-parameterized test functions will have a single test case instance,
/// and the value of this property will be an empty array for such test
/// cases.
@_spi(Experimental) @_spi(ForToolsIntegrationOnly)
public var arguments: [Argument]

init(
Expand Down Expand Up @@ -117,6 +119,7 @@ extension Test {
/// value that might be passed via this parameter to a test function. To
/// obtain the arguments of a particular ``Test/Case`` paired with their
/// corresponding parameters, use ``Test/Case/arguments``.
@_spi(Experimental) @_spi(ForToolsIntegrationOnly)
public struct Parameter: Sendable {
/// The zero-based index of this parameter within its associated test's
/// parameter list.
Expand Down
1 change: 1 addition & 0 deletions Sources/Testing/Test.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ public struct Test: Sendable {
/// an array of values describing its parameters, which may be empty if the
/// test function is non-parameterized. If this instance represents a test
/// suite, the value of this property is `nil`.
@_spi(Experimental) @_spi(ForToolsIntegrationOnly)
public var parameters: [Parameter]?

/// Whether or not this instance is a test suite containing other tests.
Expand Down
2 changes: 1 addition & 1 deletion Sources/Testing/Testing.docc/Documentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ their problems.
- ``Test(_:_:arguments:_:)``
- ``Test(_:_:arguments:)-3rzok``
- ``CustomTestArgumentEncodable``
- ``Test/Parameter``
<!-- - ``Test/Parameter`` -->
- ``Test/Case``

### Behavior validation
Expand Down
1 change: 1 addition & 0 deletions Sources/Testing/Traits/Comment.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ extension Test {
/// - traitType: The type of ``Trait`` whose comments should be returned.
///
/// - Returns: The comments found for the specified test trait type.
@_spi(Experimental)
public func comments<T>(from traitType: T.Type) -> [Comment] where T: Trait {
traits.lazy
.compactMap { $0 as? T }
Expand Down
6 changes: 3 additions & 3 deletions Sources/Testing/Traits/ConditionTrait+Macro.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ extension Trait where Self == ConditionTrait {
) -> Self {
// TODO: Semantic capture of platform name/version (rather than just a comment)
Self(
comment: message ?? "Requires \(_description(ofPlatformName: platformName, version: version))",
kind: .conditional(condition),
comments: [message ?? "Requires \(_description(ofPlatformName: platformName, version: version))"],
sourceLocation: sourceLocation
)
}
Expand Down Expand Up @@ -93,8 +93,8 @@ extension Trait where Self == ConditionTrait {
) -> Self {
// TODO: Semantic capture of platform name/version (rather than just a comment)
Self(
comment: message ?? "Obsolete as of \(_description(ofPlatformName: platformName, version: version))",
kind: .conditional(condition),
comments: [message ?? "Obsolete as of \(_description(ofPlatformName: platformName, version: version))"],
sourceLocation: sourceLocation
)
}
Expand All @@ -112,8 +112,8 @@ extension Trait where Self == ConditionTrait {
/// call it directly.
public static func __unavailable(message: Comment?, sourceLocation: SourceLocation) -> Self {
Self(
comment: message ?? "Marked @available(*, unavailable)",
kind: .unconditional(false),
comments: [message ?? "Marked @available(*, unavailable)"],
sourceLocation: sourceLocation
)
}
Expand Down
22 changes: 9 additions & 13 deletions Sources/Testing/Traits/ConditionTrait.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@
/// - ``Trait/disabled(if:_:sourceLocation:)``
/// - ``Trait/disabled(_:sourceLocation:_:)``
public struct ConditionTrait: TestTrait, SuiteTrait {
/// An optional, user-specified comment describing this trait.
public var comment: Comment?

/// An enumeration describing the kinds of conditions that can be represented
/// by an instance of this type.
enum Kind: Sendable {
Expand Down Expand Up @@ -68,6 +65,7 @@ public struct ConditionTrait: TestTrait, SuiteTrait {
/// If this trait was created using a function such as
/// ``enabled(if:_:sourceLocation:)`` that is evaluated at runtime, the value
/// of this property is `false`.
@_spi(ForToolsIntegrationOnly)
public var isConstant: Bool {
switch kind {
case .conditional:
Expand All @@ -77,6 +75,8 @@ public struct ConditionTrait: TestTrait, SuiteTrait {
}
}

public var comments: [Comment]

/// The source location where this trait was specified.
public var sourceLocation: SourceLocation

Expand All @@ -93,17 +93,13 @@ public struct ConditionTrait: TestTrait, SuiteTrait {

if !result {
let sourceContext = SourceContext(sourceLocation: sourceLocation)
throw SkipInfo(comment: commentOverride ?? comment, sourceContext: sourceContext)
throw SkipInfo(comment: commentOverride ?? comments.first, sourceContext: sourceContext)
}
}

public var isRecursive: Bool {
true
}

public var comments: [Comment] {
Array(comment)
}
}

// MARK: -
Expand Down Expand Up @@ -132,7 +128,7 @@ extension Trait where Self == ConditionTrait {
_ comment: Comment? = nil,
sourceLocation: SourceLocation = #_sourceLocation
) -> Self {
Self(comment: comment, kind: .conditional(condition), sourceLocation: sourceLocation)
Self(kind: .conditional(condition), comments: Array(comment), sourceLocation: sourceLocation)
}

/// Construct a condition trait that causes a test to be disabled if it
Expand All @@ -152,7 +148,7 @@ extension Trait where Self == ConditionTrait {
sourceLocation: SourceLocation = #_sourceLocation,
_ condition: @escaping @Sendable () async throws -> Bool
) -> Self {
Self(comment: comment, kind: .conditional(condition), sourceLocation: sourceLocation)
Self(kind: .conditional(condition), comments: Array(comment), sourceLocation: sourceLocation)
}

/// Construct a condition trait that disables a test unconditionally.
Expand All @@ -167,7 +163,7 @@ extension Trait where Self == ConditionTrait {
_ comment: Comment? = nil,
sourceLocation: SourceLocation = #_sourceLocation
) -> Self {
Self(comment: comment, kind: .unconditional(false), sourceLocation: sourceLocation)
Self(kind: .unconditional(false), comments: Array(comment), sourceLocation: sourceLocation)
}

/// Construct a condition trait that causes a test to be disabled if it
Expand All @@ -193,7 +189,7 @@ extension Trait where Self == ConditionTrait {
_ comment: Comment? = nil,
sourceLocation: SourceLocation = #_sourceLocation
) -> Self {
Self(comment: comment, kind: .conditional { !(try condition()) }, sourceLocation: sourceLocation)
Self(kind: .conditional { !(try condition()) }, comments: Array(comment), sourceLocation: sourceLocation)
}

/// Construct a condition trait that causes a test to be disabled if it
Expand All @@ -213,6 +209,6 @@ extension Trait where Self == ConditionTrait {
sourceLocation: SourceLocation = #_sourceLocation,
_ condition: @escaping @Sendable () async throws -> Bool
) -> Self {
Self(comment: comment, kind: .conditional { !(try await condition()) }, sourceLocation: sourceLocation)
Self(kind: .conditional { !(try await condition()) }, comments: Array(comment), sourceLocation: sourceLocation)
}
}