Add native iOS screenshots to integration_test #84611

jmagman · 2021-06-15T01:32:46Z

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

packages/integration_test/lib/integration_test.dart

jmagman · 2021-07-22T23:33:56Z

dev/integration_tests/ui/ios/Runner.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>


This should have been checked in before.

jmagman · 2021-07-22T23:34:25Z

packages/integration_test/example/ios/Runner.xcodeproj/project.pbxproj

@@ -475,21 +475,11 @@
 			baseConfigurationReference = 09505407E99803EF7AA92DE7 /* Pods-RunnerTests.debug.xcconfig */;
 			buildSettings = {
 				BUNDLE_LOADER = "$(TEST_HOST)";
-				CLANG_ANALYZER_NUMBER_OBJECT_CONVERSION = YES_AGGRESSIVE;


Remove unnecessary build setting overrides. I can do this in a separate PR, if you prefer.

flutter-dashboard · 2021-07-22T23:37:43Z

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

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?

stuartmorgan · 2021-07-23T16:00:56Z

packages/integration_test/ios/Classes/IntegrationTestIosTest.h

- (BOOL)testIntegrationTest:(NSString **)testResult;
+- (instancetype)initWithScreenshotDelegate:(nullable id<FLTIntegrationTestScreenshotDelegate>)delegate NS_DESIGNATED_INITIALIZER;
+
+- (BOOL)testIntegrationTest:(NSString *_Nullable *_Nullable)testResult;


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?)

This API is very weird (I just added the nullability), and the INTEGRATION_TEST_IOS_RUNNER macro is downright Swift-hostile...

stuartmorgan · 2021-07-23T16:06:54Z

packages/integration_test/ios/Classes/IntegrationTestPlugin.m

  } else {
    result(FlutterMethodNotImplemented);
  }
 }

+- (UIImage *)capturePngScreenshot {
+  UIWindow *window = [UIApplication.sharedApplication.windows filteredArrayUsingPredicate:[NSPredicate predicateWithFormat:@"keyWindow = YES"]].firstObject;


Looks like we're not running clang-format in this directory on CI? This is way over the 100 char style guide length limit.

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.

stuartmorgan · 2021-07-23T16:08:41Z

packages/integration_test/lib/_callback_io.dart

@@ -60,8 +61,8 @@ class IOCallbackManager implements CallbackManager {
    // comes up in the future. For example: `WebCallbackManager.cleanup`.
  }

-  // Whether the Flutter surface uses an Image.
-  bool _usesFlutterImage = false;
+  // Whether the Flutter surface uses an Image (Android only).


This sounds to me like using an image is Android-only, but the logic below is the reverse.

@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?

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. 👻

blasten · 2021-07-28T00:12:07Z

packages/integration_test/lib/_callback_io.dart

@@ -90,7 +91,7 @@ class IOCallbackManager implements CallbackManager {
    integrationTestChannel.setMethodCallHandler(_onMethodChannelCall);
    final List<int>? rawBytes = await integrationTestChannel.invokeMethod<List<int>>(
      'captureScreenshot',
-      null,
+      <String, dynamic>{'name': screenshot},


the above !_usesFlutterImage if statement could also check if Platform.isAndroid

blasten · 2021-07-28T00:13:06Z

packages/integration_test/lib/_callback_io.dart

-  // Whether the Flutter surface uses an Image.
-  bool _usesFlutterImage = false;
+  // Whether the Flutter surface uses an Image (Android only).
+  bool _usesFlutterImage = false || !Platform.isAndroid;

  @override
  Future<void> convertFlutterSurfaceToImage() async {


it would need to noop if !Platform.isAndroid

I meant to make it so it was never set to false on !Platform.isAndroid, let me clean this up.

blasten

LGTM % nits

jmagman

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.

jmagman · 2021-08-05T23:14:02Z

packages/integration_test/ios/Classes/IntegrationTestIosTest.h

- (BOOL)testIntegrationTest:(NSString **)testResult;
+- (instancetype)initWithScreenshotDelegate:(nullable id<FLTIntegrationTestScreenshotDelegate>)delegate NS_DESIGNATED_INITIALIZER;
+
+- (BOOL)testIntegrationTest:(NSString *_Nullable *_Nullable)testResult;


This API is very weird (I just added the nullability), and the INTEGRATION_TEST_IOS_RUNNER macro is downright Swift-hostile...

jmagman · 2021-08-05T23:19:22Z

packages/integration_test/ios/Classes/IntegrationTestPlugin.m

  } else {
    result(FlutterMethodNotImplemented);
  }
 }

+- (UIImage *)capturePngScreenshot {
+  UIWindow *window = [UIApplication.sharedApplication.windows filteredArrayUsingPredicate:[NSPredicate predicateWithFormat:@"keyWindow = YES"]].firstObject;


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.

jmagman · 2021-08-05T23:22:09Z

packages/integration_test/lib/_callback_io.dart

-  // Whether the Flutter surface uses an Image.
-  bool _usesFlutterImage = false;
+  // Whether the Flutter surface uses an Image (Android only).
+  bool _usesFlutterImage = false || !Platform.isAndroid;

  @override
  Future<void> convertFlutterSurfaceToImage() async {


I meant to make it so it was never set to false on !Platform.isAndroid, let me clean this up.

jmagman · 2021-08-06T02:43:51Z

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 will be some test harness work needed for the native side. Filed #87770.

We'll get some unit tests for this going soon.

blasten · 2021-08-06T20:09:41Z

🎉

jmagman added platform-ios iOS applications specifically f: integration_test The flutter/packages/integration_test plugin labels Jun 15, 2021

flutter-dashboard bot added the team Infra upgrades, team productivity, code health, technical debt. See also team: labels. label Jun 15, 2021

google-cla bot added the cla: yes label Jun 15, 2021

jmagman commented Jun 15, 2021

View reviewed changes

packages/integration_test/lib/integration_test.dart Outdated Show resolved Hide resolved

jmagman force-pushed the integration-test-screenshot branch 2 times, most recently from a80d1ed to bc4d3b8 Compare July 22, 2021 23:35

jmagman commented Jul 22, 2021

View reviewed changes

jmagman marked this pull request as ready for review July 22, 2021 23:37

jmagman requested review from stuartmorgan, dnfield and blasten July 23, 2021 00:53

stuartmorgan approved these changes Jul 23, 2021

View reviewed changes

blasten reviewed Jul 28, 2021

View reviewed changes

blasten approved these changes Jul 28, 2021

View reviewed changes

jmagman commented Aug 5, 2021

View reviewed changes

jmagman added 2 commits August 5, 2021 16:25

Add native iOS screenshots to integration_test

08dfdce

Android-only _usesFlutterImage

afbdcd6

Review edits

89afca0

jmagman force-pushed the integration-test-screenshot branch from 7d9f321 to 89afca0 Compare August 6, 2021 02:57

keyonghan mentioned this pull request Aug 6, 2021

LUCI presubmit check stuck pending due to buildbucket 500 #86948

Closed

jmagman added the waiting for tree to go green label Aug 6, 2021

fluttergithubbot merged commit 28dfb44 into flutter:master Aug 6, 2021

jmagman deleted the integration-test-screenshot branch August 6, 2021 20:50

jmagman mentioned this pull request Aug 11, 2021

Refactor iOS integration_test API to support Swift, dynamically add native tests #88013

Merged

speaking-in-code mentioned this pull request Aug 30, 2022

integration_test screenshots do not work on firebase android test lab #110554

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add native iOS screenshots to integration_test #84611

Add native iOS screenshots to integration_test #84611

jmagman commented Jun 15, 2021 •

edited

Loading

jmagman Jul 22, 2021

jmagman Jul 22, 2021

flutter-dashboard bot commented Jul 22, 2021

stuartmorgan left a comment

stuartmorgan Jul 23, 2021

jmagman Aug 5, 2021 •

edited

Loading

stuartmorgan Jul 23, 2021

jmagman Aug 5, 2021

stuartmorgan Jul 23, 2021

jmagman Jul 27, 2021

blasten Jul 28, 2021

blasten Jul 28, 2021

blasten Jul 28, 2021

jmagman Aug 5, 2021

blasten left a comment

jmagman left a comment

jmagman Aug 5, 2021 •

edited

Loading

jmagman Aug 5, 2021

jmagman Aug 5, 2021

jmagman commented Aug 6, 2021

blasten commented Aug 6, 2021

Add native iOS screenshots to integration_test #84611

Add native iOS screenshots to integration_test #84611

Conversation

jmagman commented Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Jul 22, 2021

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blasten left a comment

Choose a reason for hiding this comment

jmagman left a comment

Choose a reason for hiding this comment

jmagman Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman commented Aug 6, 2021

blasten commented Aug 6, 2021

jmagman commented Jun 15, 2021 •

edited

Loading

jmagman Aug 5, 2021 •

edited

Loading

jmagman Aug 5, 2021 •

edited

Loading