search

cashapp / AccessibilitySnapshot

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

Add support for SnapshotTesting as optional subspec #21

Closed Sherlouk closed 3 years ago

Sherlouk commented 3 years ago

As per https://github.com/cashapp/AccessibilitySnapshot/issues/3#issuecomment-699161416.

I've left the README and Example/Tests for now as I was wanting a bit of direction from @NickEntin.

Currently the default spec is the Uber library. If we change it, then it's a breaking change but given this is still a "beta" library that's probably acceptable but wanted to know your steer before updating all tests and documentation to reflect this.

To be done after this is released:

Let me know how you want me to proceed Nick and I'll get it sorted 👍

Thanks 😄

NickEntin commented 3 years ago

Currently the default spec is the Uber library. If we change it, then it's a breaking change but given this is still a "beta" library that's probably acceptable but wanted to know your steer before updating all tests and documentation to reflect this.

What do you think about landing this as 0.3.3 with the default subspec left as FBSnapshotTestCase, then in the next "major" beta version (0.4) we can swap over to SnapshotTesting as the default?

NickEntin commented 3 years ago

I filed #22 to track switching over the default subspec.

NickEntin commented 3 years ago

I think we're gonna have to bump the deployment target to iOS 11 for this to work. I was thinking we'd only need to set it for the subspec right now, then update the core framework when we switch the default, but it looks like CocoaPods doesn't support subspecs with different platform requirements (see CocoaPods/CocoaPods#7333).

I'll do this in a separate PR to avoid too much scope creep in this change.

Sherlouk commented 3 years ago

So it is possible to keep the deployment target as is, it'll just force a slightly older version of the SnapshotTesting library. So shouldn't matter hugely what order you merge those in.

I have just pushed up a test example - you may need to pick it up from here though as I don't have the various simulators you need to generate the images you run on CI. I tested it worked on the 11 Pro running iOS 14.

I did also have to use master on one of the dependencies as you actually resolved an issue with Xcode 12 which was preventing me testing the changes.

If you'd rather leave that all for another PR then you're equally free to revert that bad boi!

Sherlouk commented 3 years ago

Aha looks like you just got in ahead of me with #23 😄

NickEntin commented 3 years ago

Yep, just ran into that same issue 😄

Looking great so far! I'll go ahead and run through the tests and try to get all of that updated. Thanks for getting this all set up so quickly! 🙌

Sherlouk commented 3 years ago

No worries! We've been using this for a while and it's super super awesome -- can't wait for SPM support too though 😉

Thanks for looking at the CI stuff for me though 👍

NickEntin commented 3 years ago

Apologies for the delay on this. I think I'm close to getting the tests into good shape - hopefully will have something ready to go tomorrow.

NickEntin commented 3 years ago

I updated the unit tests and rebased on master to catch the dependency change around Paralayout.

NickEntin commented 3 years ago

I opened an issue (#25) proposing that we bump the deployment target to iOS 12. This would allow us to use the latest version of SnapshotTesting. Technically we only need to bump it to iOS 11, but I think usage on 11 is low enough now that it's reasonable to go straight to 12.

@Sherlouk would a minimum iOS version of 12.0 work for you? Or do you need support back further than that?

Sherlouk commented 3 years ago

Technically we support iOS 9 still in parts ðŸĪŠ we have a workaround though so I have no issues with you changing it to iOS 12!

NickEntin commented 3 years ago

Oh wow, that's a lot of versions to support. 😅 But cool! I'll go ahead push that forward - I don't think the ordering we land them in is that important.

NickEntin commented 3 years ago

Pushed a few more commits - a couple of updates to fix the CI builds (hopefully that was all we needed) and a quick lint pass to be more consistent with the other files (mostly removing trailing whitespace).

Sherlouk commented 3 years ago

Really appreciate you picking this up - sorry I haven't been around much this weekend!

NickEntin commented 3 years ago

No worries at all... thanks for getting it all set up! I think it's all looking good to me now.

@efirestone want to give this a quick look?

Sherlouk commented 3 years ago

Updated my repository pointing people here - and opened a pull request on SnapshotTesting repo to point here instead 😄

Thanks for the help and getting it merged Nick!

NickEntin commented 3 years ago

Absolutely! Thanks for agreeing to merge your extension in! I think this turned out really well. 😄

I'll go ahead and release 0.3.3 with this change, and then follow up with the iOS 12 bump and switch the default over for 0.4.0.

Sherlouk commented 3 years ago

Awesome! stuff!

Once 0.4.0 is shipped I'll rebase my SPM branch and get it all working fully. All 3 of these changes end up colliding with the work over there because of the general configuration work that's needed to get everything setup.

Once the initial SPM work is in it'll be way simpler to move forward with -- just that initial hurdle!