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.
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
Changes from 1 commit
75dd9b6
fae5f78
30bb700
147f9ce
636da1a
4d88f1c
a120b3e
dae1398
fb4f00e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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?
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
andSuiteTrait
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 theSuiteTrait
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
andTestTrait
. Do you think something is problematic about the current approach?