search

cashapp / AccessibilitySnapshot

Easy regression testing for iOS accessibility
Apache License 2.0
541 stars 67 forks source link

Bump SPM deployment target to iOS 13 #106

Closed Sherlouk closed 1 year ago

Sherlouk commented 1 year ago

This change closes #105.

I have not updated the CocoaPods specification as SnapshotTesting has dropped support for it. Rather than removing support for CocoaPods from this library, I have opted to simply not include the new update there. This means CocoaPod users will not be able to update to v1.10.0, but can at least continue to use the last functioning version. A separate conversation may be worth having around keeping CocoaPods support going on as the general community is heading in a direction without it.

This is a breaking change for SPM users, as it does drop support for iOS 12 inline with the changes upstream. Again, this does not impact CocoaPod users.

NickEntin commented 1 year ago

Since this is now basically just bumping the deployment target to iOS 13, I think we should rescope this PR to essentially be the "drop iOS 12 support" change. That way we keep consistency across the project with what we support. This consists of:

  1. Bump SPM deployment target to 13
  2. Bump CP deployment target to 13
  3. Bump sample app deployment target to 13
  4. Remove the iOS 12 build job (and associated scripts/config)
  5. Update requirements in README
  6. Remove iOS 12-specific logic for matching VO behavior (I think there's only one case in UIAccessibility+SnapshotAdditions.swift)

Does that sound reasonable?

Sherlouk commented 1 year ago

I think yes and no.

Right now this pull request is a blocker for numerous projects as folks are currently unable to use the latest version of SnapshotTesting if they also use AccessibilitySnapshot. There are important patches upstream so there is really a time critical aspect of this change.

Additionally, SnapshotTesting upstream has dropped support for CocoaPods. Bumping the CP deployment target to 13 is essentially pointless - rather the next natural step is to completely remove support for CocoaPods. This isn't a small piece of work though as it requires significant changes to the sample app and all of the CI scripts (it was where I first started).

I still think (in my opinion) this pull request is valuable on it's own in order to unblock teams who want/need to utilise the latest SnapshotTesting library. I then think there needs to be clarity on the next broader steps, in essence are you ready to drop CP, which can be handled as a separate PR.

Unrelated but the README states "We do not currently support AccessibilitySnapshot and iOSSnapshotTestCase through Swift Package Manager." which has been wrong for a while 😛

NickEntin commented 1 year ago

Apologies for the delay on this. I think we're good to merge this PR as-is (although I am going to rename it to better reflect what it's changing now) and follow up with the other steps I mentioned above.

rather the next natural step is to completely remove support for CocoaPods

I disagree here. Many folks are still using CocoaPods and we should continue to support it while we can. I suspect SnapshotTesting dropping CP means many of those developers will migrate to iOSSnapshotTestCase, but I don't think there's any reason not to support using AccessibilitySnapshot with an older version of SnapshotTesting through CP for now.

the README states "We do not currently support AccessibilitySnapshot and iOSSnapshotTestCase through Swift Package Manager." which has been wrong for a while 😛

🤦‍♂️ Oops. I really need CI for my READMEs. #110