-
Notifications
You must be signed in to change notification settings - Fork 74
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
[SWT-NNNN] Introduce API allowing traits to customize test execution #733
base: main
Are you sure you want to change the base?
[SWT-NNNN] Introduce API allowing traits to customize test execution #733
Conversation
75975cf
to
75dd9b6
Compare
@swift-ci please test Windows |
|
||
I propose the following new APIs: | ||
|
||
- A new protocol `CustomTestExecuting` with a single required `execute(...)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gerunds for protocol names are common in Objective-C, but in Swift we usually prefer an adjective form, e.g. (bikeshedding here!) CustomTestExecutable
or CustomExecutable
or CustomTestSetuppableAndTeardownable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely open to these alternate suggestions, thanks. But I'll note: as I was considering names myself, I found this in the Swift API Design Guidelines:
- Protocols that describe a capability should be named using the suffixes
able
,ible
, oring
(e.g.Equatable
,ProgressReporting
).
Based on that, I think an ing
suffix would be acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it's not wrong, just uncommon. So it's worth thinking about something more idiomatic if we can come up with something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I slightly dislike about the able
suffixes is that it makes it seem as if the work is optional. "This type is able to execute the test… but will it? Who knows!?". But if you define a trait this way, it will execute tests it's applied to, and to me the ing
conveys that a little more clearly. "This type is responsible for executing the test."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have plenty of counter-examples too where a noun is used instead: Collection
, Sequence
, Actor
… maybe it really just is CustomTestExecutor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have plenty of counter-examples too where a noun is used instead:
Collection
,Sequence
,Actor
… maybe it really just isCustomTestExecutor
.
The API Design Guidelines address that other common naming convention for protocols in the bullet above the one I quoted:
- Protocols that describe what something is should read as nouns (e.g.
Collection
).
So I think this adheres to the recommendations well: the Trait
protocol best describes what the type is, and the CustomTestExecuting
protocol describes one optional capability a type (usually, a trait) can have.
struct MockAPICredentialsTrait: TestTrait, CustomTestExecuting { | ||
func execute(_ function: @Sendable () async throws -> Void, for test: Test, testCase: Test.Case?) async throws { | ||
let mockCredentials = APICredentials(apiKey: "...") | ||
try await APICredentials.$current.withValue(mockCredentials) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We think task locals are going to be a particularly common use case, right? Perhaps we should build out a trait type that sets a task local:
@Test(.withValue(123, for: $foo)) func f() { ... }
Or even if we don't make it a trait, can we make it easier to write a trait that sets a task local with minimal boilerplate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that would be a very useful, built-in trait to have. Let's consider that separately, since it would ultimately leverage the API proposed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't need to be part of this proposal, but I think it does affect this proposal. Might be worth coming up with an example of when you'd use this stuff other than to set a task local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Briefly: not all work before/after a test is simply mutating state in that process. It may involve writing to the filesystem, or interacting with other system-wide resources. (Clearly, parallelism would need to be considered in such scenarios.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: Users can still use this facility to mutate global or static state, as long as it complies with Swift concurrency rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make sure to add this example to the discussion?
@swift-ci please test Windows |
func execute(_ function: @Sendable () async throws -> Void, for test: Test, testCase: Test.Case?) async throws | ||
} | ||
|
||
extension Trait where Self: TestExecuting { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want this to be TestTrait
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler doesn't allow that, because then it's ambiguous for types which conform to both TestTrait
and SuiteTrait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I have it now, the extension on Trait
acts as the "default" implementation, and the SuiteTrait
refines it conditionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it should be ambiguous for types that conform to both, or we should provide an additional default for them that executes once per suite or test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to break the current ability to have a trait type conform to both SuiteTrait
and TestTrait
. Do you think something is problematic about the current approach?
/// This method should either invoke `function` once before returning or throw | ||
/// an error if it is unable to perform its custom logic successfully. | ||
/// | ||
/// This method is invoked once for the test its associated trait is applied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this paragraph is now out of date? At a minimum, it should probably refer to the other function.
@stmontgomery Per our conversation offline you mentioned that order of execution of custom execution traits is well-defined to be outer-to-inner, left-to-right -- it would be great if we could explicitly add a note about that in the documentation so that folks are aware of the expected behavior! |
This includes an API proposal and code changes to introduce new API for custom traits to customize test execution.
View the API proposal for more details.
Motivation:
One of the primary motivations for the trait system in Swift Testing, as described in the vision document, is to provide a way to customize the behavior of tests which have things in common. If all the tests in a given suite type need the same custom behavior,
init
and/ordeinit
(if applicable) can be used today. But if only some of the tests in a suite need custom behavior, or tests across different levels of the suite hierarchy need it, traits would be a good place to encapsulate common logic since they can be applied granularly per-test or per-suite. This aspect of the vision for traits hasn't been realized yet, though: theTrait
protocol does not offer a way for a trait to customize the execution of the tests or suites it's applied to.Customizing a test's behavior typically means running code either before or after it runs, or both. Consolidating common set-up and tear-down logic allows each test function to be more succinct with less repetitive boilerplate so it can focus on what makes it unique.
Checklist: