Add support for specifying the UI testing screenshot behavior in a scheme test action #942
Conversation
|
I'm guessing that the Xcode 12 job was recently added? The failure looks unrelated to this PR. |
|
@brentleyjones Is there any appetite for merging this change soon? My team in particular is definitely looking forward to it. |
|
I'm on paternity leave for a while. @yonaskolb can you take a look? |
There was a problem hiding this comment.
Seeing as though these 2 booleans end up as an enum, we could also just add a simple attachmentLifetime enum to the spec instead. Thoughts?
There was a problem hiding this comment.
My thought was that the "attachment lifetime" is more of an implementation detail of the XCScheme rather than the Scheme in the project spec. From the user's perspective, the bools are a 1:1 mapping with these two checkboxes in the Xcode scheme editor. Happy to discuss this more, though.
There was a problem hiding this comment.
@yonaskolb any more thoughts on this? I'm happy to make this change if you think it's the right call.
daltonclaybrook
force-pushed
the
dalton/ui-testing-screenshots
branch
from
d9dc697 to
8a29412
Compare
daltonclaybrook
force-pushed
the
dalton/ui-testing-screenshots
branch
from
8a29412 to
27c985c
Compare
Sources/ProjectSpec/Scheme.swift
Outdated
There was a problem hiding this comment.
Could you write these values below in toJSONValue if they differ from the default
daltonclaybrook
force-pushed
the
dalton/ui-testing-screenshots
branch
from
27c985c to
4d1f51a
Compare
Sources/ProjectSpec/Scheme.swift
Outdated
| "region": region, | ||
| "coverageTargets": coverageTargets.map { $0.reference }, | ||
| "captureScreenshotsAutomatically": captureScreenshotsAutomatically, | ||
| "deleteScreenshotsWhenEachTestSucceeds": deleteScreenshotsWhenEachTestSucceeds, |
There was a problem hiding this comment.
Thanks. Sorry if it wasn't clear before, but these values should only be written to the dictionary if they are not their default value (true), like below. This is so generated specs only contain the required information
There was a problem hiding this comment.
Ah, got it! This makes perfect sense. Will make the change right away.
4 participants
Resolves #941