Skip to content
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

[snapshot] Fix compatibility with macOS #19864

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ptrkstr
Copy link

@ptrkstr ptrkstr commented Jan 26, 2022

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

Why is this change required? What problem does it solve?

  • Snapshot targets that target macOS would fail to build when including SnapshotHelper.swift because it contained symbols not available on macOS. This PR solves that problem by ensuring that macOS only sees symbols available to it.
  • Additionally screenshots were being stored in an incorrect home directory.
  • Enhancement added that allows specifying what XCUIScreenshotProviding object should be screenshotted. This is useful when taking screenshots for macOS and you'd prefer to screenshot the window and not the screen. This does not break backwards compatibility.

For conversations around Snapshot on macOS, please see:

Description

  • Added screenshotProvider: XCUIScreenshotProviding = XCUIScreen.main to open class func snapshot(_ name: String, timeWaitingForIdle timeout: TimeInterval = 20).
  • Inside if os(OSX) condition of the above mentioned function, removed call of app.typeKey(XCUIKeyboardKeySecondaryFn, modifierFlags: []) and added screenshot saving functionality.
  • Wrapped class func fixLandscapeOrientation(image: UIImage) -> UIImage with #if os(iOS) || os(watchOS) || os(tvOS) as UIImage isn't available on macOS.
  • Change macOS cache directory to URL(fileURLWithPath: "/Users/\(NSUserName())")
  • Updated screenshot.mac? to check against Apple MacBook instead of MacBook.
  • Removed self.top_space_above_device = offset['titleHeight'] # needed for centering the title from put_device_into_background as titleHeight isn't available for Mac.
  • Updated references to @config from fetch_config.

Testing Steps

@ptrkstr ptrkstr changed the title Fix SnapshotHelper.swift not building with macOS target [Snapshot] Fix SnapshotHelper.swift not building with macOS target Jan 26, 2022
@ptrkstr ptrkstr changed the title [Snapshot] Fix SnapshotHelper.swift not building with macOS target [snapshot] Fix SnapshotHelper.swift not building with macOS target Jan 26, 2022
@ptrkstr ptrkstr changed the title [snapshot] Fix SnapshotHelper.swift not building with macOS target [snapshot] Fix compatibility with macOS Jan 27, 2022
@ptrkstr ptrkstr marked this pull request as draft January 27, 2022 03:52
@ptrkstr
Copy link
Author

ptrkstr commented Mar 17, 2022

This is still in progress. Will get round to it very soon.

@joshdholtz
Copy link
Member

@ptrkstr Hey! I love this PR and just wanted to see if there was anything that you needed help with 😊

@ptrkstr
Copy link
Author

ptrkstr commented May 28, 2022

Hey @joshdholtz thanks for reaching out! I think it should be good to go for review. Maybe one additional change could be that it doesn't require a user to add 'concurrent_simulators(false)' to their snapfile and instead that's ignored if it's snapshotting for mac?

@ptrkstr
Copy link
Author

ptrkstr commented Dec 28, 2022

Hey @joshdholtz, bumping this 😌

@ptrkstr ptrkstr force-pushed the patch-2 branch 2 times, most recently from 77d6d8f to 70a5e39 Compare April 9, 2023 16:21
@MortenGregersen
Copy link
Contributor

Shouldn't we get this merged in, @joshdholtz ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants