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.

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 7 commits into from
Jan 9, 2025
Merged

Remove SPIAwareTrait and adopt TestScoping in its place #901

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
27770b9
Deprecate the SPIAwareTrait SPI protocol and adopt TestScoping in its…
stmontgomery Jan 8, 2025
f47ed31
Use a computed property in Runner
stmontgomery Jan 8, 2025
f79bbe7
Continue tweaking phrasing of ParallelizationTrait documentation
stmontgomery Jan 8, 2025
65704b5
Include bug report link
stmontgomery Jan 8, 2025
e9b9dc9
Delete obsolete SPI instead of only deprecating
stmontgomery Jan 8, 2025
c05542a
Bring back Runner.Plan.Action.RunOptions.isParallelizationEnabled sin…
stmontgomery Jan 8, 2025
3b30eb4
Simplify Runner changes more
stmontgomery Jan 8, 2025
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: 0 additions & 1 deletion Sources/Testing/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ add_library(Testing
Traits/ConditionTrait+Macro.swift
Traits/HiddenTrait.swift
Traits/ParallelizationTrait.swift
Traits/SPIAwareTrait.swift
Traits/Tags/Tag.Color.swift
Traits/Tags/Tag.Color+Loading.swift
Traits/Tags/Tag.List.swift
Expand Down
37 changes: 3 additions & 34 deletions Sources/Testing/Running/Runner.Plan.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,7 @@ extension Runner {
public enum Action: Sendable {
/// A type describing options to apply to actions of case
/// ``Runner/Plan/Action/run(options:)`` when they are run.
public struct RunOptions: Sendable, Codable {
/// Whether or not this step should be run in parallel with other tests.
///
/// By default, all steps in a runner plan are run in parallel if the
/// ``Configuration/isParallelizationEnabled`` property of the
/// configuration passed during initialization has a value of `true`.
///
/// Traits such as ``Trait/serialized`` applied to individual tests may
/// affect whether or not that test is parallelized.
///
/// ## See Also
///
/// - ``ParallelizationTrait``
public var isParallelizationEnabled: Bool
}
public struct RunOptions: Sendable, Codable {}

/// The test should be run.
///
Expand Down Expand Up @@ -65,19 +51,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 +191,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 +251,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
97 changes: 40 additions & 57 deletions Sources/Testing/Running/Runner.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ public struct Runner: Sendable {
// MARK: - Running tests

extension Runner {
/// The current configuration _while_ running.
///
/// This should be used from the functions in this extension which access the
/// current configuration. This is important since individual tests or suites
/// may have traits which customize the execution scope of their children,
/// including potentially modifying the current configuration.
private static var _configuration: Configuration {
.current ?? .init()
}

/// Apply the custom scope for any test scope providers of the traits
/// associated with a specified test by calling their
/// ``TestScoping/provideScope(for:testCase:performing:)`` function.
Expand All @@ -70,7 +80,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 +113,13 @@ 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.
/// - 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?,
_ 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 +128,7 @@ extension Runner {
}

// If not parallelizing, wait after each task.
if !isParallelizationEnabled {
if !_configuration.isParallelizationEnabled {
try await taskGroup.waitForAll()
}
}
Expand All @@ -137,12 +139,6 @@ 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`.
///
/// - Throws: Whatever is thrown from the test body. Thrown errors are
/// normally reported as test failures.
Expand All @@ -157,7 +153,7 @@ 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?>) async throws {
// Exit early if the task has already been cancelled.
try Task.checkCancellation()

Expand All @@ -168,18 +164,18 @@ extension Runner {

// Determine what action to take for this step.
if let step = stepGraph.value {
Event.post(.planStepStarted(step), for: (step.test, nil), configuration: configuration)
Event.post(.planStepStarted(step), for: (step.test, nil), configuration: _configuration)

// Determine what kind of event to send for this step based on its action.
switch step.action {
case .run:
Event.post(.testStarted, for: (step.test, nil), configuration: configuration)
Event.post(.testStarted, for: (step.test, nil), configuration: _configuration)
shouldSendTestEnded = true
case let .skip(skipInfo):
Event.post(.testSkipped(skipInfo), for: (step.test, nil), configuration: configuration)
Event.post(.testSkipped(skipInfo), for: (step.test, nil), configuration: _configuration)
shouldSendTestEnded = false
case let .recordIssue(issue):
Event.post(.issueRecorded(issue), for: (step.test, nil), configuration: configuration)
Event.post(.issueRecorded(issue), for: (step.test, nil), configuration: _configuration)
shouldSendTestEnded = false
}
} else {
Expand All @@ -188,30 +184,30 @@ extension Runner {
defer {
if let step = stepGraph.value {
if shouldSendTestEnded {
Event.post(.testEnded, for: (step.test, nil), configuration: configuration)
Event.post(.testEnded, for: (step.test, nil), configuration: _configuration)
}
Event.post(.planStepEnded(step), for: (step.test, nil), configuration: configuration)
Event.post(.planStepEnded(step), for: (step.test, nil), configuration: _configuration)
}
}

if let step = stepGraph.value, case .run = step.action {
await Test.withCurrent(step.test) {
_ = await Issue.withErrorRecording(at: step.test.sourceLocation, configuration: configuration) {
_ = await Issue.withErrorRecording(at: step.test.sourceLocation, configuration: _configuration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update withErrorRecording to just pull the current task-local value? Do we ever use it in a way that needs to take any other value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given where I landed with other changes I don't think I'll need this right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I agree in principle we could make that utility infer the current configuration, just not sure it's needed for this PR

try await _applyScopingTraits(for: step.test, testCase: nil) {
// Run the test function at this step (if one is present.)
if let testCases = step.test.testCases {
try await _runTestCases(testCases, within: step)
}

// 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 +218,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 +230,13 @@ 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.
///
/// - 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?>) 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 +265,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 @@ -298,13 +281,13 @@ extension Runner {
///
/// 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) async throws {
// Apply the configuration's test case filter.
let testCases = testCases.lazy.filter { testCase in
configuration.testCaseFilter(testCase, step.test)
_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 @@ -320,19 +303,19 @@ extension Runner {
///
/// 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) async throws {
// Exit early if the task has already been cancelled.
try Task.checkCancellation()

Event.post(.testCaseStarted, for: (step.test, testCase), configuration: configuration)
Event.post(.testCaseStarted, for: (step.test, testCase), configuration: _configuration)
defer {
Event.post(.testCaseEnded, for: (step.test, testCase), configuration: configuration)
Event.post(.testCaseEnded, for: (step.test, testCase), configuration: _configuration)
}

await Test.Case.withCurrent(testCase) {
let sourceLocation = step.test.sourceLocation
await Issue.withErrorRecording(at: sourceLocation, configuration: configuration) {
try await withTimeLimit(for: step.test, configuration: configuration) {
await Issue.withErrorRecording(at: sourceLocation, configuration: _configuration) {
try await withTimeLimit(for: step.test, configuration: _configuration) {
try await _applyScopingTraits(for: step.test, testCase: testCase) {
try await testCase.body()
}
Expand All @@ -342,7 +325,7 @@ extension Runner {
comments: [],
sourceContext: .init(backtrace: .current(), sourceLocation: sourceLocation)
)
issue.record(configuration: configuration)
issue.record(configuration: _configuration)
}
}
}
Expand Down Expand Up @@ -399,7 +382,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
34 changes: 15 additions & 19 deletions Sources/Testing/Traits/ParallelizationTrait.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,36 +11,32 @@
/// A type that affects whether or not a test or suite is parallelized.
///
/// When added to a parameterized test function, this trait causes that test to
/// run its cases serially instead of in parallel. When applied to a
/// non-parameterized test function, this trait has no effect. When applied to a
/// test suite, this trait causes that suite to run its contained test functions
/// and sub-suites serially instead of in parallel.
/// run its cases serially instead of in parallel. When added to a
/// non-parameterized test function, this trait has no effect.
///
/// 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.)
/// When added to a test suite, this trait causes that suite to run its
/// contained test functions (including their cases, when parameterized) and
/// sub-suites serially instead of in parallel. Any children of sub-suites are
/// also run serially.
///
/// 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)'. Please file a bug report at https://github.com/swiftlang/swift-testing/issues/new")
}

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

Expand Down
Loading