Skip to content

Remove SPIAwareTrait and adopt TestScoping in its place #901

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

Merged
merged 7 commits into from
Jan 9, 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
29 changes: 9 additions & 20 deletions Sources/Testing/Running/Runner.Plan.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,13 @@ extension Runner {
/// ## See Also
///
/// - ``ParallelizationTrait``
public var isParallelizationEnabled: Bool
@available(*, deprecated, message: "The 'isParallelizationEnabled' property is deprecated and no longer used. Its value is always false.")
public var isParallelizationEnabled: Bool {
get {
false
}
set {}
}
}

/// The test should be run.
Expand Down Expand Up @@ -65,19 +71,6 @@ extension Runner {
return true
}
}

/// Whether or not this action enables parallelization.
///
/// If this action is of case ``run(options:)``, the value of this
/// property equals the value of its associated
/// ``RunOptions/isParallelizationEnabled`` property. Otherwise, the value
/// of this property is `nil`.
var isParallelizationEnabled: Bool? {
if case let .run(options) = self {
return options.isParallelizationEnabled
}
return nil
}
}

/// A type describing a step in a runner plan.
Expand Down Expand Up @@ -218,7 +211,7 @@ extension Runner.Plan {
// Convert the list of test into a graph of steps. The actions for these
// steps will all be .run() *unless* an error was thrown while examining
// them, in which case it will be .recordIssue().
let runAction = Action.run(options: .init(isParallelizationEnabled: configuration.isParallelizationEnabled))
let runAction = Action.run(options: .init())
var testGraph = Graph<String, Test?>()
var actionGraph = Graph<String, Action>(value: runAction)
for test in tests {
Expand Down Expand Up @@ -278,11 +271,7 @@ extension Runner.Plan {
// `SkipInfo`, the error should not be recorded.
for trait in test.traits {
do {
if let trait = trait as? any SPIAwareTrait {
try await trait.prepare(for: test, action: &action)
} else {
try await trait.prepare(for: test)
}
try await trait.prepare(for: test)
} catch let error as SkipInfo {
action = .skip(error)
break
Expand Down
92 changes: 48 additions & 44 deletions Sources/Testing/Running/Runner.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ extension Runner {
///
/// - Throws: Whatever is thrown by `body` or by any of the
/// ``TestScoping/provideScope(for:testCase:performing:)`` function calls.
private func _applyScopingTraits(
private static func _applyScopingTraits(
for test: Test,
testCase: Test.Case?,
_ body: @escaping @Sendable () async throws -> Void
Expand Down Expand Up @@ -103,21 +103,18 @@ extension Runner {
///
/// - Parameters:
/// - sequence: The sequence to enumerate.
/// - step: The plan step that controls parallelization. If `nil`, or if its
/// ``Runner/Plan/Step/action`` property is not of case
/// ``Runner/Plan/Action/run(options:)``, the
/// ``Configuration/isParallelizationEnabled`` property of this runner's
/// ``configuration`` property is used instead to determine if
/// parallelization is enabled.
/// - configuration: The configuration with which the elements of `sequence`
/// should be enumerated. This function uses the configuration to
/// determine whether parallelization is enabled. The default value is the
/// current configuration, if any, or else the default configuration.
/// - body: The function to invoke.
///
/// - Throws: Whatever is thrown by `body`.
private func _forEach<E>(
private static func _forEach<E>(
in sequence: some Sequence<E>,
for step: Plan.Step?,
configuration: Configuration = .current ?? .init(),
_ body: @Sendable @escaping (E) async throws -> Void
) async throws where E: Sendable {
let isParallelizationEnabled = step?.action.isParallelizationEnabled ?? configuration.isParallelizationEnabled
try await withThrowingTaskGroup(of: Void.self) { taskGroup in
for element in sequence {
// Each element gets its own subtask to run in.
Expand All @@ -126,7 +123,7 @@ extension Runner {
}

// If not parallelizing, wait after each task.
if !isParallelizationEnabled {
if !configuration.isParallelizationEnabled {
try await taskGroup.waitForAll()
}
}
Expand All @@ -137,12 +134,9 @@ extension Runner {
///
/// - Parameters:
/// - stepGraph: The subgraph whose root value, a step, is to be run.
/// - depth: How deep into the step graph this call is. The first call has a
/// depth of `0`.
/// - lastAncestorStep: The last-known ancestral step, if any, of the step
/// at the root of `stepGraph`. The options in this step (if its action is
/// of case ``Runner/Plan/Action/run(options:)``) inform the execution of
/// `stepGraph`.
/// - configuration: The configuration with which the step should run. The
/// default value is the current configuration, if any, or else the
/// default configuration.
///
/// - Throws: Whatever is thrown from the test body. Thrown errors are
/// normally reported as test failures.
Expand All @@ -157,7 +151,10 @@ extension Runner {
/// ## See Also
///
/// - ``Runner/run()``
private func _runStep(atRootOf stepGraph: Graph<String, Plan.Step?>, depth: Int, lastAncestorStep: Plan.Step?) async throws {
private static func _runStep(
atRootOf stepGraph: Graph<String, Plan.Step?>,
configuration: Configuration = .current ?? .init()
) async throws {
// Exit early if the task has already been cancelled.
try Task.checkCancellation()

Expand Down Expand Up @@ -204,14 +201,14 @@ extension Runner {
}

// Run the children of this test (i.e. the tests in this suite.)
try await _runChildren(of: stepGraph, depth: depth, lastAncestorStep: lastAncestorStep)
try await _runChildren(of: stepGraph)
}
}
}
} else {
// There is no test at this node in the graph, so just skip down to the
// child nodes.
try await _runChildren(of: stepGraph, depth: depth, lastAncestorStep: lastAncestorStep)
try await _runChildren(of: stepGraph)
}
}

Expand All @@ -222,7 +219,7 @@ extension Runner {
///
/// - Returns: The source location of the root node of `stepGraph`, or of the
/// first descendant node thereof (sorted by source location.)
private func _sourceLocation(of stepGraph: Graph<String, Plan.Step?>) -> SourceLocation? {
private static func _sourceLocation(of stepGraph: Graph<String, Plan.Step?>) -> SourceLocation? {
if let result = stepGraph.value?.test.sourceLocation {
return result
}
Expand All @@ -234,26 +231,19 @@ extension Runner {
/// Recursively run the tests that are children of a given plan step.
///
/// - Parameters:
/// - stepGraph: The subgraph whose root value, a step, is to be run.
/// - depth: How deep into the step graph this call is. The first call has a
/// depth of `0`.
/// - lastAncestorStep: The last-known ancestral step, if any, of the step
/// at the root of `stepGraph`. The options in this step (if its action is
/// of case ``Runner/Plan/Action/run(options:)``) inform the execution of
/// `stepGraph`.
/// - stepGraph: The subgraph whose root value, a step, will be used to
/// find children to run.
/// - configuration: The configuration with which the children of `stepGraph`
/// will be run. The default value is the current configuration, if any,
/// or else the default configuration.
///
/// - Throws: Whatever is thrown from the test body. Thrown errors are
/// normally reported as test failures.
private func _runChildren(of stepGraph: Graph<String, Plan.Step?>, depth: Int, lastAncestorStep: Plan.Step?) async throws {
// Figure out the last-good step, either the one at the root of `stepGraph`
// or, if it is nil, the one passed into this function. We need to track
// this value in case we run into sparse sections of the graph so we don't
// lose track of the recursive `isParallelizationEnabled` property in the
// runnable steps' options.
let stepOrAncestor = stepGraph.value ?? lastAncestorStep

let isParallelizationEnabled = stepOrAncestor?.action.isParallelizationEnabled ?? configuration.isParallelizationEnabled
let childGraphs = if isParallelizationEnabled {
private static func _runChildren(
of stepGraph: Graph<String, Plan.Step?>,
configuration: Configuration = .current ?? .init()
) async throws {
let childGraphs = if configuration.isParallelizationEnabled {
// Explicitly shuffle the steps to help detect accidental dependencies
// between tests due to their ordering.
Array(stepGraph.children)
Expand Down Expand Up @@ -282,8 +272,8 @@ extension Runner {
}

// Run the child nodes.
try await _forEach(in: childGraphs, for: stepOrAncestor) { _, childGraph in
try await _runStep(atRootOf: childGraph, depth: depth + 1, lastAncestorStep: stepOrAncestor)
try await _forEach(in: childGraphs) { _, childGraph in
try await _runStep(atRootOf: childGraph)
}
}

Expand All @@ -292,19 +282,26 @@ extension Runner {
/// - Parameters:
/// - testCases: The test cases to be run.
/// - step: The runner plan step associated with this test case.
/// - configuration: The configuration with which the test cases should run.
/// The default value is the current configuration, if any, or else the
/// default configuration.
///
/// - Throws: Whatever is thrown from a test case's body. Thrown errors are
/// normally reported as test failures.
///
/// If parallelization is supported and enabled, the generated test cases will
/// be run in parallel using a task group.
private func _runTestCases(_ testCases: some Sequence<Test.Case>, within step: Plan.Step) async throws {
private static func _runTestCases(
_ testCases: some Sequence<Test.Case>,
within step: Plan.Step,
configuration: Configuration = .current ?? .init()
) async throws {
// Apply the configuration's test case filter.
let testCases = testCases.lazy.filter { testCase in
configuration.testCaseFilter(testCase, step.test)
}

try await _forEach(in: testCases, for: step) { testCase in
try await _forEach(in: testCases) { testCase in
try await _runTestCase(testCase, within: step)
}
}
Expand All @@ -314,13 +311,20 @@ extension Runner {
/// - Parameters:
/// - testCase: The test case to run.
/// - step: The runner plan step associated with this test case.
/// - configuration: The configuration with which the test case should run.
/// The default value is the current configuration, if any, or else the
/// default configuration.
///
/// - Throws: Whatever is thrown from the test case's body. Thrown errors
/// are normally reported as test failures.
///
/// This function sets ``Test/Case/current``, then invokes the test case's
/// body closure.
private func _runTestCase(_ testCase: Test.Case, within step: Plan.Step) async throws {
private static func _runTestCase(
_ testCase: Test.Case,
within step: Plan.Step,
configuration: Configuration = .current ?? .init()
) async throws {
// Exit early if the task has already been cancelled.
try Task.checkCancellation()

Expand Down Expand Up @@ -399,7 +403,7 @@ extension Runner {

await withTaskGroup(of: Void.self) { [runner] taskGroup in
_ = taskGroup.addTaskUnlessCancelled {
try? await runner._runStep(atRootOf: runner.plan.stepGraph, depth: 0, lastAncestorStep: nil)
try? await _runStep(atRootOf: runner.plan.stepGraph)
}
await taskGroup.waitForAll()
}
Expand Down
27 changes: 12 additions & 15 deletions Sources/Testing/Traits/ParallelizationTrait.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,28 @@
/// test suite, this trait causes that suite to run its contained test functions
/// and sub-suites serially instead of in parallel.
///
/// This trait is recursively applied: if it is applied to a suite, any
/// parameterized tests or test suites contained in that suite are also
/// serialized (as are any tests contained in those suites, and so on.)
/// If this trait is applied to a suite, any test functions or test suites
Copy link
Contributor

Choose a reason for hiding this comment

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

It's important to note that parameterized test functions are serialized (as opposed to just function A, function B, function C get serialized.)

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'll try to tweak the wording more. What I was attempting to clarify with this re-wording is that regular, non-parameterized test functions within a suite which has .serialized are also serialized alongside any sub-suites which are their peers. I found the existing wording a bit unclear about the handling of non-parameterized test functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recall this being difficult to get right the last time too—might want to ping @iamleeg & friends for assistance?

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've pushed a new phrasing in the mean time

/// contained in that suite are also serialized (as are any tests contained in
/// those suites, and so on.)
///
/// This trait does not affect the execution of a test relative to its peers or
/// to unrelated tests. This trait has no effect if test parallelization is
/// globally disabled (by, for example, passing `--no-parallel` to the
/// `swift test` command.)
///
/// To add this trait to a test, use ``Trait/serialized``.
public struct ParallelizationTrait: TestTrait, SuiteTrait {
public var isRecursive: Bool {
true
}
}
public struct ParallelizationTrait: TestTrait, SuiteTrait {}

// MARK: - SPIAwareTrait
// MARK: - TestScoping

@_spi(ForToolsIntegrationOnly)
extension ParallelizationTrait: SPIAwareTrait {
public func prepare(for test: Test, action: inout Runner.Plan.Action) async throws {
if case var .run(options) = action {
options.isParallelizationEnabled = false
action = .run(options: options)
extension ParallelizationTrait: TestScoping {
public func provideScope(for test: Test, testCase: Test.Case?, performing function: @Sendable () async throws -> Void) async throws {
guard var configuration = Configuration.current else {
throw SystemError(description: "There is no current Configuration when attempting to provide scope for test '\(test.name)'")
}

configuration.isParallelizationEnabled = false
try await Configuration.withCurrent(configuration, perform: function)
}
}

Expand Down
3 changes: 2 additions & 1 deletion Sources/Testing/Traits/SPIAwareTrait.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
/// This protocol refines ``Trait`` in various ways that require the use of SPI.
/// Ideally, such requirements will be promoted to API when their design
/// stabilizes.
@_spi(Experimental) @_spi(ForToolsIntegrationOnly)
@available(*, deprecated, message: "The SPIAwareTrait protocol is deprecated and no longer used.")
@_spi(ForToolsIntegrationOnly)
public protocol SPIAwareTrait: Trait {
/// Prepare to run the test to which this trait was added.
///
Expand Down
10 changes: 0 additions & 10 deletions Tests/TestingTests/Traits/ParallelizationTraitTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,6 @@

@Suite("Parallelization Trait Tests", .tags(.traitRelated))
struct ParallelizationTraitTests {
@Test(".serialized trait is recursively applied")
func serializedTrait() async {
var configuration = Configuration()
configuration.isParallelizationEnabled = true
let plan = await Runner.Plan(selecting: OuterSuite.self, configuration: configuration)
for step in plan.steps {
#expect(step.action.isParallelizationEnabled == false, "Step \(step) should have had parallelization disabled")
}
}

@Test(".serialized trait serializes parameterized test")
func serializesParameterizedTestFunction() async {
var configuration = Configuration()
Expand Down