swift-snapshot-testing icon indicating copy to clipboard operation
swift-snapshot-testing copied to clipboard

Use iOS 16 ImageRenderer to snapshot SwiftUI Views

Open john-flanagan opened this issue 2 years ago • 15 comments

The new ImageRenderer type in iOS 16 is purpose built for converting SwiftUI views into images. Seems like a great fit for snapshot testing.

~~I took the approach of using ImageRenderer whenever possible for the .image strategy because it seems likely to produce the best results by default, but I'd be happy to discuss another approach.~~

I wan't able to get the existing images for testSwiftUIView_iOS to pass with or without my change on any devices I tried on either Xcode 13.5 or Xcode 14. Does it require some specific setup to get the tests passing?

john-flanagan avatar Sep 12 '22 01:09 john-flanagan

I'm wondering why the .sizeThatFits layout is generating a 5px wider image file?

I couldn't figure out how to generate a matching snapshots to the recorded images even on the main branch without my changes.

The Makefile looks like the tests were designed to be run on an iPhone 11 Pro Max, but even on the versions of Xcode the CI runs on I couldn't get the tests to pass on main.

The CI only runs make test-swift so it hasn't been validating the iOS tests.

john-flanagan avatar Sep 13 '22 14:09 john-flanagan

FYI using this renderer fixes a crash we are seeing when snapshotting a SwiftUI Map on an iOS 16 simulator.

DavidBrunow avatar Sep 19 '22 19:09 DavidBrunow

@stephencelis I figured since I wasn't trying to match the API of .image it'd make sense to embrace an API that more directly mirrors ImageRenderer's properties

One of the notable changes is that .imageRenderer doesn't match the simulator's display scale by default, instead opting for ImageRenderers default behavior of rendering a 1 pixel per point.

Thoughts?

john-flanagan avatar Sep 22 '22 03:09 john-flanagan

@john-flanagan Will try to take this for a spin soon. Also I think you brought up .imageRender as being a nice name for this, and I agree that it looks nicer in practice:

assertSnapshot(matching: SwiftUIView(), as: .imageRender)

What do you think of renaming?

stephencelis avatar Sep 22 '22 16:09 stephencelis

What do you think of renaming?

I'm good with .imageRender as well. I'll make the update

john-flanagan avatar Sep 23 '22 01:09 john-flanagan

@stephencelis (or anyone else following along) I've been playing with the renderer more and I'd love your thoughts on a couple things:

  1. I think the isOpaque argument should be dropped from the current implementation and the system default background should be applied to all of the layouts.

I think this will be more intuitive for consumers since it will behave like SwiftUI Previews where transparent views still display over a default background.

Also the isOpaque argument renders transparent pixels as black which doesn't seem like a good fit for this purpose.

  1. Currently we pass .unspecified as the default ProposedViewSize, which is also the default behavior of ImageRenderer. As a result a view using .sizeThatFits layout will be allowed to grow as large as it wants in any direction by default. For example, a really long Text view will take up as much horizontal length as it needs in order to display as a single line of text, even if that goes well beyond the user's selected simulator.

Obviously users can opt-in to device sized rendering using a .device(...) layout, but what do you think of of making the the proposedSize parameter optional and falling back to ProposedViewSize(UIScreen.main.bounds.size) when it's absent?

That way the default behavior would again roughly match SwiftUI Previews where .previewLayout(.sizeThatFits) attempts to fit the view on the current simulator's screen size, but allow it to shrink if it can.

Both .device and .fixedSize layouts would override the proposed size since they're laid out with an explicit .frame.

The drawback is that the default output of .imageRender would then be dependent on the user's selected simulator, but it would behave more like .image and SwiftUI Previews

john-flanagan avatar Sep 24 '22 23:09 john-flanagan

Hey! Thanks for the great work, I tested it with some views, and I found that it doesn't seem to work with SwiftUI.ScrollViews.

for example:

   assertSnapshot(
            matching: ScrollView { Text("Hello world") } ,
            as: .imageRender(layout: .device(config: .iPhone12), proposedSize: .unspecified)
        )

will give a white image as a result :(

Edit: Ok actually, it makes sense when reading the ImageRenderer Documentation

I guess It still makes sense to add this but maybe the documentation should specify that this won't work with any Views backed by UIKit

HugoSay avatar Oct 07 '22 16:10 HugoSay

Hey @stephencelis, I think this PR is in pretty good shape. Mind giving it a once over?

john-flanagan avatar Nov 15 '22 04:11 john-flanagan

@stephencelis 👋 looks like just an approval is not enough, it asking for "First-time contributors need a maintainer to approve running workflows" :O

nikita-leonov avatar Dec 08 '22 19:12 nikita-leonov

Hi @stephencelis I can't merge this branch. Is there anything else you're looking for out of this PR? image

john-flanagan avatar Dec 21 '22 15:12 john-flanagan

I just tried this out and could verify this to work. It helped to workaround the problem, I've encountered here. @stephencelis What's stopping this from getting merged and being shipped?

mrackwitz avatar Jan 20 '23 11:01 mrackwitz

@mrackwitz Sorry for the delay in action and communication! We do try to get to core issues and bugs with SnapshotTesting when we have time, but when it comes to bugs and contributions around strategies that can be replaced in user land, we unfortunately have to put those on the back burner right now. New strategies becomes additional surface area that we might have to be on alert for, and changes to existing strategies become vectors for regressions.

We do plan on returning to SnapshotTesting this year with a revamped 2.0.0 that will be modernized, more powerful, and easier to build a plug-in ecosystem around. We also want folks to feel freer to share their own strategies as standalone libraries that can be dropped in rather than everyone being on hold/wait for us to merge them.

@john-flanagan On that note, what do you think of shipping this in a standalone package for now (that we can reference on the README), and when we return to spend more energy iterating on this library later this year we can revisit.

Regardless of a standalone library being available, though, the additions in this PR should be easily pulled into your own projects if they help you!

stephencelis avatar Jan 20 '23 20:01 stephencelis

@stephencelis, I happy to create a new package for the imageRender strategy. I totally get your reasoning.

One thing to consider though is that some companies make it tough to add new third party dependencies, and adding them from arbitrary Github users can be even more challenging. I don't know how much effort you're interesting in investing in accommodating annoying corporate policies (I doubt very much, and I don't blame you), but there's definitely an advantage to having a wide set of strategies ship with the library itself for some consumers of the library.

That said I just pushed up this repository: https://github.com/john-flanagan/SnapshotTestingImageRender

john-flanagan avatar Jan 21 '23 02:01 john-flanagan

Thanks @stephencelis for getting back quickly and no offense from my side. I totally see getting things falling through the cracks given the sheer amount of repos you've up here. So I also totally understand your reasoning to switch gears re:community contributions and larger API surface area. Indeed I had just pulled the imageRender strategy in a simplified version into a separate file. But thanks @john-flanagan for following up swiftly, then we can depend on that instead.

mrackwitz avatar Jan 22 '23 12:01 mrackwitz

Any update on getting this in? Should be able to support macOS 13+ as well

andrewtheis avatar Nov 01 '23 19:11 andrewtheis