Add native iOS screenshots to integration_test#84611
Conversation
jmagman
added
platform-ios
flutter-dashboard
Bot
added
the
c: contributor-productivity
a80d1ed to
bc4d3b8
Compare
There was a problem hiding this comment.
This should have been checked in before.
There was a problem hiding this comment.
Remove unnecessary build setting overrides. I can do this in a separate PR, if you prefer.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM with nits. Shouldn't we have a test of this functionality that screenshots a dummy plugin and compares it to a golden image though?
There was a problem hiding this comment.
Could we get a comment on this weird API describing its behavior while you are touching it? (Or even better, could we make this an idiomatic ObjC method that returns either an NSString or nil?)
There was a problem hiding this comment.
This API is very weird (I just added the nullability), and the INTEGRATION_TEST_IOS_RUNNER macro is downright Swift-hostile...
There was a problem hiding this comment.
Looks like we're not running clang-format in this directory on CI? This is way over the 100 char style guide length limit.
There was a problem hiding this comment.
I don't think there are any formatters (even dartfmt) running on flutter/flutter (plus I hate how clang formatter mangles Obj-C blocks). Let me shorten these up.
There was a problem hiding this comment.
This sounds to me like using an image is Android-only, but the logic below is the reverse.
There was a problem hiding this comment.
@blasten What does Whether the Flutter surface uses an Image mean in this context? I think that the surface is frozen, the screenshot taken, and then it's unfrozen on Android?
There was a problem hiding this comment.
oh - yes. Image really means that Flutter is using this class https://github.com/flutter/engine/blob/master/shell/platform/android/io/flutter/embedding/android/FlutterImageView.java. 👻
There was a problem hiding this comment.
the above !_usesFlutterImage if statement could also check if Platform.isAndroid
There was a problem hiding this comment.
it would need to noop if !Platform.isAndroid
There was a problem hiding this comment.
I meant to make it so it was never set to false on !Platform.isAndroid, let me clean this up.
jmagman
left a comment
jmagman
left a comment
There was a problem hiding this comment.
LGTM with nits. Shouldn't we have a test of this functionality that screenshots a dummy plugin and compares it to a golden image though?
I think the next step for @blasten after I commit this was to set up the screenshot integration tests. But I should also write some iOS native tests.
There was a problem hiding this comment.
This API is very weird (I just added the nullability), and the INTEGRATION_TEST_IOS_RUNNER macro is downright Swift-hostile...
There was a problem hiding this comment.
I don't think there are any formatters (even dartfmt) running on flutter/flutter (plus I hate how clang formatter mangles Obj-C blocks). Let me shorten these up.
There was a problem hiding this comment.
I meant to make it so it was never set to false on !Platform.isAndroid, let me clean this up.
There will be some test harness work needed for the native side. Filed #87770. We'll get some unit tests for this going soon. |
7d9f321 to
89afca0
Compare
|
🎉 |
4 participants
Implement the native iOS screenshot side of #84472. Added an optional "name" argument for the "captureScreenshot" method.
When run as a native Xcode test, the screenshots are saved to the Xcode test results bundle in addition to the bytes being passed back over the channel.
Screenshot is the app window, not including the status bar, etc.
iOS screenshot prototype of #83856