-
Notifications
You must be signed in to change notification settings - Fork 2.5k
TaskLocal test trait #3329
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
base: main
Are you sure you want to change the base?
TaskLocal test trait #3329
Changes from 14 commits
4481ab4
d9a994c
1e023f2
f7ed72d
087d945
e1e7c8d
d11e8e1
f749024
612a21b
8313095
e2ae5ad
9cf7893
2c20426
30b37ed
fe9ba30
aa3ae4c
883510e
7c08931
cf1ba5e
92781aa
7e4f0b6
b0dff58
aca6e45
54fe282
5901b56
02bd8f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,163 @@ | ||||||
| # TaskLocal test trait | ||||||
|
|
||||||
| * Proposal: [ST-NNNN](NNNN-task-local-test-trait.md) | ||||||
| * Authors: [Brandon Williams](https://github.com/mbrandonw), [Stephen Celis](https://github.com/stephencelis) | ||||||
| * Review Manager: TBD | ||||||
| * Status: **Awaiting review** | ||||||
| * Implementation: [swiftlang/swift-testing#NNNNN](https://github.com/swiftlang/swift-testing/compare/main...pointfreeco:swift-testing:task-local-test-trait) | ||||||
| * Review: ([pitch](https://forums.swift.org/...)) | ||||||
|
|
||||||
| # Introduction | ||||||
|
|
||||||
| A `.taskLocal` test trait is added to Testing that allows binding a value to a task local for a suite | ||||||
| or test. | ||||||
|
|
||||||
| # Motivation | ||||||
|
|
||||||
| In [ST-0007], test scoping traits were introduced to allow wrapping the execution | ||||||
| of tests so that code could be run before and after each test. The main | ||||||
| motivation for this feature is overriding [`TaskLocal`s][SE-0311: TaskLocal] for tests, and the | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, this still isn't showing the link in the rendered version of the document. Might be a markdown syntax quirk
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might work better as a standard markdown link instead of the brackets link. |
||||||
| proposal mentions a [future direction][task-local-future-direction] to add a | ||||||
| convenience trait specifically for binding task locals. This proposal adds that | ||||||
| convenience. | ||||||
|
|
||||||
| [SE-0331: TaskLocal]: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0311-task-locals.md | ||||||
| [ST-0007]: https://github.com/swiftlang/swift-evolution/blob/main/proposals/testing/0007-test-scoping-traits.md | ||||||
| [task-local-future-direction]: https://github.com/swiftlang/swift-evolution/blob/main/proposals/testing/0007-test-scoping-traits.md#convenience-trait-for-setting-task-locals | ||||||
|
|
||||||
| There are two primary motivations for adding this convenience: | ||||||
|
|
||||||
| * Task locals are by far the [most common] reason to define custom `TestScoping` | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
not sure why the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The brackets link just below to some context: https://github.com/search?q=testscoping+language%3ASwift+&type=code We just found that a lot of the public |
||||||
| conformances. However, it takes quite a bit of repetitive work to define such | ||||||
| conformances. For example, a trait to bind a boolean to a task local looks like this: | ||||||
|
|
||||||
| [most common]: https://github.com/search?q=testscoping+language%3ASwift+&type=code | ||||||
|
|
||||||
| ```swift | ||||||
| struct IsEnabledTrait: SuiteTrait, TestScoping, TestTrait { | ||||||
| let isEnabled: Bool | ||||||
| let isRecursive = true | ||||||
| func provideScope( | ||||||
| for test: Test, | ||||||
| testCase: Test.Case?, | ||||||
| performing function: () async throws -> Void | ||||||
| ) async throws { | ||||||
| try await FeatureFlags.$isEnabled.withValue(isEnabled) { | ||||||
| try await function() | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| extension Trait where Self == IsEnabledTrait { | ||||||
| static func isEnabled(_ isEnabled: Bool) -> Self { | ||||||
| Self(isEnabled: isEnabled) | ||||||
| } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| Ideally, we can drastically streamline overriding task locals in test with a dedicated convenience | ||||||
| trait. | ||||||
|
|
||||||
| * When a task local is defined in a reusable library, the library maintainer cannot easily define a | ||||||
| test trait for overriding the task local. Non-test targets cannot access Testing symbols, and so | ||||||
| such a helper needs to be defined in a dedicated "test support" library. This introduces more | ||||||
| inconvenience when shipping task locals in libraries. A dedicated `.taskLocal` trait allows one | ||||||
| to bind values to task locals in tests without creating a dedicated test support library. | ||||||
|
|
||||||
| # Proposed solution | ||||||
|
|
||||||
| A `.taskLocal` trait will be defined in Testing that allows overriding a task local for a single | ||||||
| test or a whole suite: | ||||||
|
|
||||||
| ```swift | ||||||
| @Suite(.taskLocal(FeatureFlags.$isEnabled, true)) | ||||||
| struct MySuite { | ||||||
| // ... | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| # Detailed design | ||||||
|
|
||||||
| A new type will be defined to conform to `TestScoping`, as well as a static function for ease of | ||||||
| constructing the trait: | ||||||
|
|
||||||
| ```swift | ||||||
| extension Trait { | ||||||
| /// Constructs a trait that binds a task local value for the duration of a test | ||||||
| /// or suite. | ||||||
| /// | ||||||
| /// - Parameters: | ||||||
| /// - taskLocal: The task local to bind the value to. | ||||||
| /// - value: The value to set. | ||||||
| /// | ||||||
| /// ```swift | ||||||
| /// @Suite(.taskLocal($myValue, 42)) | ||||||
| /// struct MyTests { | ||||||
| /// // ... | ||||||
| /// } | ||||||
| /// ``` | ||||||
| /// | ||||||
| /// - Note: The task local must be defined outside the test target where the trait is used. | ||||||
|
stephencelis marked this conversation as resolved.
Outdated
|
||||||
| public static func taskLocal<Value: Sendable>( | ||||||
| _ taskLocal: TaskLocal<Value>, | ||||||
| _ value: Value | ||||||
| ) -> Self | ||||||
| where Self == TaskLocalTrait<Value> { | ||||||
| TaskLocalTrait(taskLocal: taskLocal, value: value) | ||||||
| } | ||||||
|
stephencelis marked this conversation as resolved.
Outdated
|
||||||
| } | ||||||
|
|
||||||
| /// A type that binds a task local value for the scope of a test. | ||||||
|
stephencelis marked this conversation as resolved.
|
||||||
| /// | ||||||
| /// To add this trait to a test, use ``Trait/taskLocal(_:_:)``. | ||||||
| public struct TaskLocalTrait<Value: Sendable>: SuiteTrait, TestScoping, TestTrait { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor stylistic thing, but I'd list |
||||||
| public var isRecursive: Bool { true } | ||||||
|
|
||||||
| fileprivate let taskLocal: TaskLocal<Value> | ||||||
| fileprivate let value: Value | ||||||
|
stephencelis marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| public func provideScope( | ||||||
| for test: Test, | ||||||
| testCase: Test.Case?, | ||||||
| performing function: @concurrent () async throws -> Void | ||||||
| ) async throws { | ||||||
| try await taskLocal.withValue(value, operation: function) | ||||||
| } | ||||||
|
stephencelis marked this conversation as resolved.
Outdated
|
||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| An important caveat to note (and is detailed in the documentation) is that the task local being | ||||||
| bound _must_ be defined outside the test target. One cannot bind a task local value like so: | ||||||
|
|
||||||
| ```swift | ||||||
| @TaskLocal var isEnabled = false | ||||||
|
|
||||||
| @Test(.taskLocal($isEnabled, true)) // 🛑 Cannot find '$isEnabled' in scope | ||||||
| func test() { | ||||||
| // ... | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| This is because currently the `@Test` macro cannot see the `$isEnabled` symbol created by the | ||||||
| `@TaskLocal` macro. There are use cases for defining task locals in the test target along side tests | ||||||
| that want to override them, but it's not common, and you always have the option to move the task | ||||||
| local to a separate target. | ||||||
|
|
||||||
| ## Source compatibility | ||||||
|
|
||||||
| The proposed APIs are purely additive. | ||||||
|
|
||||||
| ## Alternatives considered | ||||||
|
|
||||||
| A few other spellings have been proposed: | ||||||
|
|
||||||
| * `.set($isEnabled, true)` | ||||||
| * `.withValue(true, for: $isEnabled)` | ||||||
| * `.binding($isEnabled, to: true)` | ||||||
|
|
||||||
| We could also decide to not add the trait since technically it is possible to implement its | ||||||
| functionality manually for each task local. | ||||||
|
|
||||||
| ## Future directions | ||||||
|
|
||||||
| If Swift macros gain the ability to see symbols defined by other macros, we will be able to drop | ||||||
| the restriction that task locals be defined outside the test target. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand where this comes from, but the phrasing of "…we will be able to drop the restriction…" could be a little confusing: the API itself isn't imposing any such restriction, it's a consequence of current language restrictions outside the scope of this proposal. I'd suggest tweaking this by saying something like: if this macro limitation were resolved, then this proposed API would become usable in more contexts/situations. The thing I'm looking to clarify is that no further action or change to the API would be required; it would "only" require a change to the language or macro system. |
||||||
Uh oh!
There was an error while loading. Please reload this page.