Unskip Impeller iOS scenario tests #55012

jmagman · 2024-09-06T20:19:23Z

The iOS golden scenario tests are supposed to run on Skia and Impeller, but are actually running on Skia twice.

Issue 1: script-side

Regression from #46329.
Currently, the tests log "Running simulator tests with Impeller" but then you see that it's not:

Running simulator tests with Impeller
...
2024-09-06 13:38:07.692810-0700 Scenarios[73857:470266] [IMPORTANT:flutter/shell/common/shell.cc(456)] [Action Required] The application opted out of Impeller by either using the --no-enable-impeller flag or FLTEnableImpeller=false plist flag. This option is going to go away in an upcoming Flutter release. Remove the explicit opt-out. If you need to opt-out, report a bug describing the issue.

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8737514337106072353/+/u/test:_Scenario_App_Integration_Tests/stdout

INFOPLIST_FILE="Scenarios/Info_Impeller.plist" isn't passed into the xcodebuild any more. I confirmed #46329 caused this by inserting a comment in the middle of the xcodebuild command, which caused the last argument after the comment to not be passed in.

Issue 2: test-side

Regression from #45093.
The logic deciding whether to use Skia or Impeller goldens is incorrect since FLTEnableImpeller is set in the app Info.plist, not the UI test, so the bundle should be "dev.flutter.Scenarios". That means even if the script was passing in the right Info.plist, the tests would fail since they would have compared the Skia screenshots.

This PR

Once Turn off software rendering in iOS scenario golden tests #55016 merges (edit: done), these tests will no longer be running with the software renderer. Remove all the test skips so they all run on Impeller.
Add the missing Impeller golden screenshots.
Update the default Info.plist to use Impeller, so if another script mishap happens, it will default to testing Impeller twice, and not Skia. Add a Skia Info.plist variant instead.
Update the test logic to check the right bundle ID Info.plist to decide whether to compare against the Skia or Impeller screenshots. Prefer Impeller so if another test-side mishap happens, it will also default to testing Impeller and not Skia. It will only use the Skia goldens if the bool is set in the Info.plist, and that bool is NO (not just if it's missing).
All this made the now-default Impeller tests pass, but when passing in INFOPLIST_FILE="Scenarios/Info_Skia.plist" the app wasn't launching, with the error:

Scenarios encountered an error (Failed to install or launch the test runner. (Underlying Error: Simulator device returned an error for the requested operation. Failed to create promise. (Underlying Error: Failed to create app extension placeholder for /Users/chrome-bot/Library/Developer/Xcode/DerivedData/Scenarios-aypjgouuxctxctfazxalyegcximf/Build/Products/Debug-iphonesimulator/Scenarios.app/PlugIns/ScenariosShare.appex. Failed to create promise. (Underlying Error: Failed to set placeholder attributes dev.flutter.Scenarios.ScenariosShare. Failed to create promise. (Underlying Error: extensionDictionary must be set in placeholder attributes for an app extension placeholder. Invalid placeholder attributes.)))))

This is identical to the error in #53717. I finally realized it was because the ScenariosShare app extension requires specific extension keys in its plist, which weren't present in the app Info.plist at Scenarios/Info_Skia.plist. So I changed the INFOPLIST_FILE path to $(TARGET_NAME)/Info_Skia.plist so it would resolve to target-specific copies like Scenarios/ScenariosShare/Info_Skia.plist. That meant I had to add a few more copies where they didn't exist in the target path.

Dependent on #55016 landing.
Fixes flutter/flutter#131888

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jmagman · 2024-09-06T20:58:57Z

Hmm, this passed without golden changes, I'm suspicious.

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8737514337106072353/+/u/test:_Scenario_App_Integration_Tests/stdout

Uh oh, is this not testing impeller at all?

Running simulator tests with Impeller
...
2024-09-06 13:38:07.692810-0700 Scenarios[73857:470266] [IMPORTANT:flutter/shell/common/shell.cc(456)] [Action Required] The application opted out of Impeller by either using the --no-enable-impeller flag or FLTEnableImpeller=false plist flag. This option is going to go away in an upcoming Flutter release. Remove the explicit opt-out. If you need to opt-out, report a bug describing the issue.

Follow-up to #45093. Remove software rendering for the golden/screenshot compare iOS scenario tests. Throw an exception if this flag is passed into the test app. Delete `RenderingSelectionTest`. This did require updating some screenshots, so there are changes. Note, these golden tests are still not running on Impeller, but I'm going to follow-up in #55012. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

jmagman · 2024-09-07T03:01:34Z

testing/scenario_app/ios/Scenarios/Scenarios/TextPlatformView.m

+  self = [super init];
+  if (self) {


Fix a stray analysis warning.

jmagman · 2024-09-07T03:01:38Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/GoldenImage.m

@@ -99,7 +99,7 @@ - (BOOL)compareGoldenToImage:(UIImage*)image rmesThreshold:(double)rmesThreshold
  return YES;
 }

-NS_INLINE NSString* _platformName() {
+NS_INLINE NSString* _platformName(void) {


Fix a stray analysis warning.

hellohuanlin

LGTM!

…154883) flutter/engine@c18ecf9...656117f 2024-09-09 [email protected] Unskip Impeller iOS scenario tests (flutter/engine#55012) 2024-09-09 [email protected] Roll Skia from 7ad1252da14f to 7e659d5538fa (2 revisions) (flutter/engine#55050) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Follow-up to flutter#45093. Remove software rendering for the golden/screenshot compare iOS scenario tests. Throw an exception if this flag is passed into the test app. Delete `RenderingSelectionTest`. This did require updating some screenshots, so there are changes. Note, these golden tests are still not running on Impeller, but I'm going to follow-up in flutter#55012. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

The iOS golden scenario tests are supposed to run on Skia and Impeller, but are actually running on Skia twice. #### Issue 1: script-side Regression from flutter#46329. Currently, the tests log "Running simulator tests with Impeller" but then you see that it's not: ``` Running simulator tests with Impeller ... 2024-09-06 13:38:07.692810-0700 Scenarios[73857:470266] [IMPORTANT:flutter/shell/common/shell.cc(456)] [Action Required] The application opted out of Impeller by either using the --no-enable-impeller flag or FLTEnableImpeller=false plist flag. This option is going to go away in an upcoming Flutter release. Remove the explicit opt-out. If you need to opt-out, report a bug describing the issue. ``` https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8737514337106072353/+/u/test:_Scenario_App_Integration_Tests/stdout `INFOPLIST_FILE="Scenarios/Info_Impeller.plist"` isn't passed into the xcodebuild any more. I confirmed flutter#46329 caused this by inserting a comment in the middle of the `xcodebuild` command, which caused the last argument after the comment to not be passed in. #### Issue 2: test-side Regression from flutter#45093. [The logic deciding whether to use Skia or Impeller goldens is incorrect](https://github.com/flutter/engine/blob/397987aedeb4ed41ec945fcbe9d347e579513bf4/testing/scenario_app/ios/Scenarios/ScenariosUITests/GoldenTestManager.m#L84-L90) since `FLTEnableImpeller` is set in the app Info.plist, not the UI test, so the bundle should be "dev.flutter.Scenarios". That means even if the script was passing in the right Info.plist, the tests would fail since they would have compared the Skia screenshots. #### This PR 1. Once flutter#55016 merges (edit: done), these tests will no longer be running with the software renderer. Remove all the test skips so they all run on Impeller. 2. Add the missing Impeller golden screenshots. 3. Update the default Info.plist to use Impeller, so if another script mishap happens, it will default to testing Impeller twice, and not Skia. Add a Skia Info.plist variant instead. 4. Update the test logic to check the right bundle ID Info.plist to decide whether to compare against the Skia or Impeller screenshots. Prefer Impeller so if another test-side mishap happens, it will also default to testing Impeller and not Skia. It will only use the Skia goldens if the bool is set in the Info.plist, and that bool is NO (not just if it's missing). 5. All this made the now-default Impeller tests pass, but when passing in `INFOPLIST_FILE="Scenarios/Info_Skia.plist"` the app wasn't launching, with the error: > Scenarios encountered an error (Failed to install or launch the test runner. (Underlying Error: Simulator device returned an error for the requested operation. Failed to create promise. (Underlying Error: Failed to create app extension placeholder for /Users/chrome-bot/Library/Developer/Xcode/DerivedData/Scenarios-aypjgouuxctxctfazxalyegcximf/Build/Products/Debug-iphonesimulator/Scenarios.app/PlugIns/ScenariosShare.appex. Failed to create promise. (Underlying Error: Failed to set placeholder attributes dev.flutter.Scenarios.ScenariosShare. Failed to create promise. (Underlying Error: extensionDictionary must be set in placeholder attributes for an app extension placeholder. Invalid placeholder attributes.))))) This is identical to the error in flutter#53717. I finally realized it was because the ScenariosShare app extension requires specific extension keys in its plist, which weren't present in the app Info.plist at `Scenarios/Info_Skia.plist`. So I changed the `INFOPLIST_FILE` path to `$(TARGET_NAME)/Info_Skia.plist` so it would resolve to target-specific copies like `Scenarios/ScenariosShare/Info_Skia.plist`. That meant I had to add a few more copies where they didn't exist in the target path. Dependent on flutter#55016 landing. Fixes flutter/flutter#131888 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

This was referenced Sep 6, 2024

[Impeller] Re-enable the Scenario iOS UI golden tests. flutter/flutter#131888

Closed

Turn off software rendering in iOS scenario golden tests #55016

Merged

jmagman added 8 commits September 6, 2024 19:56

Unskip Impeller iOS scenario tests

adabfb2

Force it to use Impeller even in the 'Skia' parts

c773657

Fix impeller bundle ID check to app

4891b49

Update impeller goldens

90eb018

Default to testing Impeller unless explicitly set to Skia

4e6035a

plist path

90c4b44

self

cd4b091

Dummy plist

6574194

jmagman force-pushed the scenario-impeller-tests branch from 749d990 to 6574194 Compare September 7, 2024 02:58

jmagman commented Sep 7, 2024

View reviewed changes

Swap order

fbe2d9e

jmagman mentioned this pull request Sep 7, 2024

Redirect run_ios_tests.sh to run_ios_tests.dart. #53717

Merged

jmagman added 3 commits September 6, 2024 22:14

Add missing Info_Skia.plists

1a499c3

Add dummy plist comments

6ed4163

Readd shell script comment

1df4b71

jmagman marked this pull request as ready for review September 9, 2024 16:13

jmagman marked this pull request as draft September 9, 2024 16:13

run_ios_tests

bdf2a1c

jmagman requested review from cbracken and hellohuanlin September 9, 2024 20:38

jmagman marked this pull request as ready for review September 9, 2024 20:38

hellohuanlin approved these changes Sep 9, 2024

View reviewed changes

jmagman added e: impeller autosubmit Merge PR when tree becomes green via auto submit App labels Sep 9, 2024

auto-submit bot merged commit 656117f into flutter:main Sep 9, 2024
35 checks passed

jmagman deleted the scenario-impeller-tests branch September 9, 2024 21:47

engine-flutter-autoroll mentioned this pull request Sep 9, 2024

Roll Flutter Engine from c18ecf9e4000 to 656117fc8ba9 (2 revisions) flutter/flutter#154883

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2024

656117fc8 Unskip Impeller iOS scenario tests (flutter/engine#55012)

4c5537a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Unskip Impeller iOS scenario tests #55012

Unskip Impeller iOS scenario tests #55012

Uh oh!

jmagman commented Sep 6, 2024 •

edited

Loading

Uh oh!

jmagman commented Sep 6, 2024

Uh oh!

jmagman Sep 7, 2024

Uh oh!

jmagman Sep 7, 2024

Uh oh!

hellohuanlin left a comment

Uh oh!

Uh oh!

Uh oh!

Unskip Impeller iOS scenario tests #55012

Unskip Impeller iOS scenario tests #55012

Uh oh!

Conversation

jmagman commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue 1: script-side

Issue 2: test-side

This PR

Pre-launch Checklist

Uh oh!

jmagman commented Sep 6, 2024

Uh oh!

jmagman Sep 7, 2024

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 7, 2024

Choose a reason for hiding this comment

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmagman commented Sep 6, 2024 •

edited

Loading