TaskLocal test trait#1764
Conversation
mbrandonw
requested review from
briancroom,
grynspan,
harlanhaskins,
jerryjrchen and
stmontgomery
as code owners
| /// } | ||
| /// ``` | ||
| /// | ||
| /// - Note: The task local must be defined outside the test target where the trait is used. |
There was a problem hiding this comment.
Documentation comments should avoid the passive voice per Apple's and Swift's documentation style guides.
Per our own convention, put discussion including callouts after the brief, parameters, returns, and throws sections.
| /// Constructs a trait that overrides a task local value for the duration of a test | ||
| /// or suite. | ||
| /// | ||
| /// ```swift |
There was a problem hiding this comment.
An example like this is probably not necessary, but if you want to include it then it needs to go after a paragraph or two describing the trait in more detail. It won't render well in DocC immediately after the brief.
| /// | ||
| /// To add this trait to a test, use ``Trait/taskLocal(_:_:)``. | ||
| public struct TaskLocalTrait<Value: Sendable>: SuiteTrait, TestScoping, TestTrait { | ||
| public var isRecursive: Bool { true } |
There was a problem hiding this comment.
Because task locals are already implicitly applied recursively to subtasks, there's no need for this property's value to be true. Having it true here just causes us to call withValue repeatedly, which does not have a meaningful effect on test code.
| public struct TaskLocalTrait<Value: Sendable>: SuiteTrait, TestScoping, TestTrait { | ||
| public var isRecursive: Bool { true } | ||
|
|
||
| fileprivate let taskLocal: TaskLocal<Value> |
There was a problem hiding this comment.
Please provide documentation comments for these two stored properties. As well, because this is a struct, please declare them var rather than let.
|
Additional action items:
|
grynspan
added
enhancement
|
GitHub's multiple assignee feature is broken, I guess. |
| /// - Note: You must define the task local outside the test target where the trait is used. | ||
| public static func taskLocal<Value: Sendable>( | ||
| _ taskLocal: TaskLocal<Value>, | ||
| _ value: Value |
There was a problem hiding this comment.
I recommend making the value parameter an @autoclosure so that its evaluation can be deferred until the point where provideScope() is called, if it gets called at all:
| _ value: Value | |
| _ value: @autoclosure @escaping @Sendable () throws -> Value |
A couple reasons:
- If the test is skipped (e.g. it has a
.disabledtrait) thevalueexpression will never be evaluated. That can be useful if the evaluation has observable side effects you only want to occur if the test actually runs, especially if that work is expensive. - The value obtained lazily during
provideScope()can be released and discarded once the test finishes, which again can be important if the value occupies significant memory or otherwise needs to be deallocated once it's no longer needed.
Making this a lazily-evaluated stored property on TaskLocalTrait would also align it more closely with ConditionTrait, which stores its condition as a closure which gets evaluated lazily during planning.
If you take this change, I recommend documenting those semantics in the DocC for the .taskLocal() function, and discussing the reasoning for using @autoclosure in the accompanying proposal.
There was a problem hiding this comment.
If you take this change, I recommend documenting those semantics in the DocC for the .taskLocal() function, and discussing the reasoning for using @autoclosure in the accompanying proposal.
I've made the change for @autoclosure, but on this point I'm not sure what to document. I couldn't find any examples in the Swift ecosystem of autoclosures that are documented. Usually the docs treat the argument as a regular value and do not mention the autoclosure at all, including enabled(if:)/disabled(if:).
There was a problem hiding this comment.
The aspect I would encourage you to document is the lifetime guarantees that lazy evaluation via @autoclosure will provide. You could say something like,
/// - Parameters:
/// ...
/// - value: The value to bind to `taskLocal`.
///
/// This value will only be evaluated if the test this
// trait is applied to runs, and it will be unbound from
/// the task local and released once the trait is finished
/// providing scope for the test.
|
Keep in mind async autoclosures are broken. |
| public func provideScope( | ||
| for test: Test, | ||
| testCase: Test.Case?, | ||
| performing function: @concurrent () async throws -> Void |
There was a problem hiding this comment.
We don't have @concurrent on other provideScope() implementations in the library. The other implementations do have @Sendable though; can we get away with only @Sendable ?
| } | ||
|
|
||
| #if DEBUG | ||
| @TaskLocal var dummyLocal = false |
There was a problem hiding this comment.
Could you include a DocC comment about why this is here? (It's a convenient place to put it for use in an accompanying test, because it needs to be in a separate module)
Also, style nit: we generally outdent code within top-level #ifs to column 0
4 participants
Adds a
.taskLocaltest trait for overriding task locals in testsMotivation:
Proposal here: swiftlang/swift-evolution#3329
Checklist: