-
Notifications
You must be signed in to change notification settings - Fork 19
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
Support test screenshot configuration options #82
base: main
Are you sure you want to change the base?
Conversation
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.
Would you mind creating the counterpart PR in Tuist? We usually don't merge PRs in XcodeGraph
until the counterpart PR in Tuist is approved.
captureScreenshotsAutomatically: Bool, | ||
deleteScreenshotsWhenEachTestSucceeds: Bool, |
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.
This technically constitutes a breaking change which we are trying to avoid.
I'd consider adding the new options to TestingOptions
instead.
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'd suggest to model these options more closer to how they are modeled in XcodeProj: https://github.com/tuist/XcodeProj/blob/9f26d78d72ef40dd2e35f624bbcde1e3b28762cf/Sources/XcodeProj/Scheme/XCScheme%2BTestAction.swift#L42
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'd consider adding the new options to TestingOptions instead.
got it. Will make the change right away.
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'd suggest to model these options more closer to how they are modeled in XcodeProj:
https://github.com/tuist/XcodeProj/blob/9f26d78d72ef40dd2e35f624bbcde1e3b28762cf/Sources/XcodeProj/Scheme/XCScheme%2BTestAction.swift#L42
What do you think about this comment? I believe that structuring variables closer to the xcodeproj model as you suggested might make it less user-friendly from an user perspective. (There are no such Boolean values for captureScreenshotsAutomatically
and deleteScreenshotsWhenEachTestSucceeds
)
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.
captureScreenshotsAutomatically
definitely should be an enum as you can select between screenshots and recordings. That wasn't the case when that comment was posted.
Also, XcodeGraph
is not something Tuist users directly interact with and I think we benefit from keeping the way we structure things somewhat similar. So, I'd still be for aligning with XcodeProj
.
Add configuration for scroll performance testing
Background
When measuring scroll performance using FPS in Xcode, the current test configuration options in Tuist don't provide accurate measurement environments. For example, without disabling certain settings, what should be 60 FPS is incorrectly measured as 30 FPS.
Proposed Solution
I propose to add two test configuration options:
captureScreenshotsAutomatically
: This will allow users to disable automatic screenshots during testing, which is crucial for accurate scroll performance measurements.deleteScreenshotsWhenEachTestSucceeds
: This option will work in conjunction withcaptureScreenshotsAutomatically
to configure AttachmentLifeTime (currently not utilized in Tuist).These additions will help developers obtain more accurate performance measurements and better manage test artifacts.
Impact
Note
The following PR will be submitted to Tuist based on this work