feat: Add handling for Swift Testing Only Parallelization #871
Conversation
|
@kelvinharron we can merge this one once the CI is green ✅ |
| @@ -6,7 +6,7 @@ public extension XCScheme { | |||
| // MARK: - Attributes | |||
|
|
|||
| public var skipped: Bool | |||
| public var parallelizable: Bool | |||
| public var parallelizable: Parallelizable | |||
There was a problem hiding this comment.
This may constitute as breaking change :/
Options:
- Introduce a new type
parallelizableValue: Parallelizable(or some better name 😅) and keepparallelizable: Boolas a Bool + mark deprecated - it can translate the to and from underlying enum property - Ensure a major version is released of XcodeProj
Interesting ideas but may not be sufficient:
- Leverage
ExpressibleByBooleanLiteralhttps://developer.apple.com/documentation/swift/expressiblebybooleanliteral - that may mitigate any literal usage but will still break any clients using a boolean variable or reading this boolean variable :/
There was a problem hiding this comment.
I think it's worth investigating using the ExpressibleByBooleanLiteral to make the migration easier. Technically speaking, it won't be sufficient, so let's make a new major version of XcodeProj. I think for this library, having major versions more often than let's say for the Tuist CLI is fine.
There was a problem hiding this comment.
What if we do something like
@available(...) -> Deprecate with a warn
public var parallelizable: Bool { ... } // as-is
public var parallelization: TestParallelization { ... }There was a problem hiding this comment.
+1 this gets it a bit closer to the nomenclature in the Xcode UI
There was a problem hiding this comment.
Hey @kelvinharron - what do you think about this ?
If you want an extra hand I'm happy to finalize this PR so we can land it into XcodeProj. LMK!
Thanks :)
There was a problem hiding this comment.
Thanks for the feedback @kwridan, keep me right on the most recent changes as it feels a little loose with two properties. This PR extends more so than replaces now, so we're handling both mapping functions from attributes and inits as before, with the introduction of TestParallelization option.
There was a problem hiding this comment.
The var parallelizable can be a computed property over the enum swiftTestingOnly (which is the default case today, right?)
@available(...) -> Deprecate with a warn
public var parallelizable: Bool {
get { parallelization == .swiftTestingOnly }
set { parallelization = newValue ? swiftTestingOnly : .none }
}
public var parallelization: TestParallelization { ... }
that way it's just a backward-compatiblemirror. WDYT?
There was a problem hiding this comment.
Ace @freak4pc. I've updated and added some tests to reason about the computed var behaviour. Please take a look and let me know if that's expected.
There was a problem hiding this comment.
@kwridan @fortmarek let me know if you have any additional feedback and I can close this out. I think we're encountering some failures on CI with these changes I can look at.
|
Anything preventing us from moving forward? Would love to have swift-testing at our disposal :D |
There was a problem hiding this comment.
This looks good! I'd wait with the merge until the Tuist PR counterpart is approved as well: tuist/tuist#6898
| self.skippedTests = skippedTests | ||
| } | ||
|
|
||
| @available(*, deprecated, message: "Use init with Parallelization argument instead") | ||
| public init(skipped: Bool, | ||
| parallelizable: Bool = false, |
There was a problem hiding this comment.
Let's remove the default value, so the deprecation warning is triggered only when parallelizable is specified.
Thanks Marek. I still haven't found a way to reliably prove out the tests for that PR meaning its a non starter. @freak4pc care to look at the PR and see if you have any ideas? Some more context on Slack here. |
|
@all-contributors add @kelvinharron for code |
|
I've put up a pull request to add @kelvinharron! 🎉 |
I don't have extra ideas apart from what I posted. Feel free to simplify the acceptance test if needed. However, I think we can merge this one without having a Tuist acceptance test in place once the CI is ✅ |
pepicrft
changed the title
… available in TestableReference with deprecation warning to maintain backwards caompatability
pepicrft
force-pushed
the
add-swifttesting-parallelizable-handling
branch
from
411947a to
0926e9d
Compare
|
@kelvinharron your changes are now part of XcodeProj 8.25.0 |
Fantastic. Thank you for tidying this up and for everyone's feedback. My next change will be measured in days, not weeks and months 😄 |
Related to this Tuist issue
Short description 📝
This PR introduces XcodeProj Parallelization handling for the new Swift Testing option. This will enable us to expose the option to Tuist
TestingOptionsallowing users to specify the option they need, as the default handling in Xcode 16 for Parallelization is going to benefit the few users who have adopted Swift Testing.I also removed two dead links from the References section as I was going through the reading material.
Solution 📦
From changing the values manually in a real project, I gathered:
With guidance from @fortmarek , I updated the handling of the attributes to cover each individual case.
I've set it as draft as I need guidance on how to prove this handling outside of unit tests. Please let me know what I could be missing and how to verify the output. Thanks.
Implementation 👩💻👨💻