Simplify the configuration option for synchronous test isolation. #747
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR replaces `Configuration.isMainActorIsolationEnforced` with a new property `Configuration.defaultIsolationContext` which can be set to any actor, not just the main actor. This property is then used as the isolation context for synchronous test functions that are not otherwise actor-isolated. The typical use case for this property would be to set it to `MainActor.shared` so that synchronous test functions run on the main actor, but a testing tool might want to instead run them all on another actor or serial executor for performance (or other) reasons. This change isn't just speculative: it allows us to simplify the macro-generated thunk for synchronous test functions so that it does not need to emit the body of the thunk twice (once for the main actor, once for non-isolated.) This change wasn't possible before `#isolation` was introduced, but we'd already added `Configuration.isMainActorIsolationEnforced` when that happened so the timing didn't line up. By rewriting this part of the macro expansion, we're able to toss a bunch of support code that isn't needed anymore. Yay!
grynspan
added
enhancement
grynspan
requested review from
stmontgomery,
dennisweissmann,
briancroom,
SeanROlszewski and
suzannaratcliff
as code owners
|
@swift-ci test |
briancroom
approved these changes
Oct 4, 2024
|
I probably need to make the closure sendable. That sounds like a problem for Monday Jonathan. |
…tIsolationContext` property and use a local closure that the compiler can see is only called once, solving the consuming/borrowing problem.
|
Saturday Jonathan swooped in and rethinked/rethunked it. |
|
@swift-ci test |
…ip that bit back out
|
The line diff has shrunk, but mostly because I've added a bunch of comments trying to explain these shenanigans. |
stmontgomery
reviewed
Oct 7, 2024
stmontgomery
reviewed
Oct 7, 2024
stmontgomery
reviewed
Oct 7, 2024
stmontgomery
reviewed
Oct 7, 2024
stmontgomery
approved these changes
Oct 7, 2024
|
@swift-ci test |
|
@swift-ci test |
2 tasks
stmontgomery
added a commit
that referenced
this pull request
Oct 10, 2024
…ed by @test (#759) Small cleanup from the changes in #747: Remove an unintentional parameter label which resulted in a parameter being named `isolated_` when it should be just `_`. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
|
@grynspan |
That property isn't something users of the testing library are intended to have access to control. What problem are you trying to solve? This conversation may be better in the Development > Swift Testing subcategory on the forums. |
2 tasks
stmontgomery
added a commit
that referenced
this pull request
Apr 9, 2025
…n code for synchronous test functions (#1067) This fixes a compilation error in code expanded from the `@Test` macro when it's attached to a synchronous (i.e. non-`async`) test function in a context where there is a concrete type named `Actor`. For example, the following code reproduces the error: ```swift // In MyApp public class Actor {} // In test code import Testing import MyApp // ❌ 'any' has no effect on concrete type 'Actor' // - 'isolated' parameter type 'Actor?' does not conform to 'Actor' or 'DistributedActor' @test func example() /* No 'async' */ {} ``` The macro code includes an unqualified reference to a type by that name, but it's intended to refer to the protocol in Swift's `_Concurrency` module. The fix is to ensure the macro's reference to this protocol is fully-qualified with a module name. This was first reported on the Swift Forums in https://forums.swift.org/t/error-isolated-parameter-type-actor-does-not-conform-to-actor-or-distributedactor/79190. This bug was introduced in #747, which first landed in Swift 6.1 and Xcode 16.3. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
stmontgomery
added a commit
to stmontgomery/swift-testing
that referenced
this pull request
Apr 11, 2025
…n code for synchronous test functions (swiftlang#1067) This fixes a compilation error in code expanded from the `@Test` macro when it's attached to a synchronous (i.e. non-`async`) test function in a context where there is a concrete type named `Actor`. For example, the following code reproduces the error: ```swift // In MyApp public class Actor {} // In test code import Testing import MyApp // ❌ 'any' has no effect on concrete type 'Actor' // - 'isolated' parameter type 'Actor?' does not conform to 'Actor' or 'DistributedActor' @test func example() /* No 'async' */ {} ``` The macro code includes an unqualified reference to a type by that name, but it's intended to refer to the protocol in Swift's `_Concurrency` module. The fix is to ensure the macro's reference to this protocol is fully-qualified with a module name. This was first reported on the Swift Forums in https://forums.swift.org/t/error-isolated-parameter-type-actor-does-not-conform-to-actor-or-distributedactor/79190. This bug was introduced in swiftlang#747, which first landed in Swift 6.1 and Xcode 16.3. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
stmontgomery
added a commit
to stmontgomery/swift-testing
that referenced
this pull request
Apr 11, 2025
…n code for synchronous test functions (swiftlang#1067) This fixes a compilation error in code expanded from the `@Test` macro when it's attached to a synchronous (i.e. non-`async`) test function in a context where there is a concrete type named `Actor`. For example, the following code reproduces the error: ```swift // In MyApp public class Actor {} // In test code import Testing import MyApp // ❌ 'any' has no effect on concrete type 'Actor' // - 'isolated' parameter type 'Actor?' does not conform to 'Actor' or 'DistributedActor' @test func example() /* No 'async' */ {} ``` The macro code includes an unqualified reference to a type by that name, but it's intended to refer to the protocol in Swift's `_Concurrency` module. The fix is to ensure the macro's reference to this protocol is fully-qualified with a module name. This was first reported on the Swift Forums in https://forums.swift.org/t/error-isolated-parameter-type-actor-does-not-conform-to-actor-or-distributedactor/79190. This bug was introduced in swiftlang#747, which first landed in Swift 6.1 and Xcode 16.3. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces
Configuration.isMainActorIsolationEnforcedwith a new propertyConfiguration.defaultIsolationContextwhich can be set to any actor, not just the main actor. This property is then used as the isolation context for synchronous test functions that are not otherwise actor-isolated.The typical use case for this property would be to set it to
MainActor.sharedso that synchronous test functions run on the main actor, but a testing tool might want to instead run them all on another actor or serial executor for performance (or other) reasons.This change isn't just speculative: it allows us to simplify the macro-generated thunk for synchronous test functions so that it does not need to emit the body of the thunk twice (once for the main actor, once for non-isolated.) This change wasn't possible before
#isolationwas introduced, but we'd already addedConfiguration.isMainActorIsolationEnforcedwhen that happened so the timing didn't line up.By rewriting this part of the macro expansion, we're able to toss a bunch of support code that isn't needed anymore. Yay!
Checklist: