This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Turn off software rendering in iOS scenario golden tests #55016
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmagman
changed the title
zanderso
approved these changes
Sep 7, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 7, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 7, 2024
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Sep 7, 2024
…154769) flutter/engine@419fb8c...7bf1169 2024-09-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[engine] reland: always post tasks for platform channel responses. (#55006)" (flutter/engine#55022) 2024-09-07 [email protected] Turn off software rendering in iOS scenario golden tests (flutter/engine#55016) 2024-09-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[skwasm] use temporary RawPaint objects (#54917)" (flutter/engine#55018) 2024-09-06 [email protected] [engine] reland: always post tasks for platform channel responses. (flutter/engine#55006) 2024-09-06 [email protected] Roll Skia from 4786936b4c0c to 06cd203d0607 (2 revisions) (flutter/engine#55014) 2024-09-06 [email protected] [multiple] Avoid new `unreachable_switch_default` warning. (flutter/engine#54996) 2024-09-06 [email protected] [skwasm] use temporary RawPaint objects (flutter/engine#54917) 2024-09-06 [email protected] [DisplayList] use DlScalar, DlPoint, DlRect exclusively in DlOpReceiver methods (flutter/engine#54982) 2024-09-06 [email protected] Roll Skia from f38ea0134dba to 4786936b4c0c (4 revisions) (flutter/engine#55013) 2024-09-06 [email protected] [Impeller] opt into exp canvas. (flutter/engine#54913) 2024-09-06 [email protected] Roll Skia from 8f62a6a4a299 to f38ea0134dba (4 revisions) (flutter/engine#55008) 2024-09-06 [email protected] [impeller] fake image for fake tests. (flutter/engine#54974) 2024-09-06 [email protected] Roll Skia from 6ad117bd2efe to 8f62a6a4a299 (1 revision) (flutter/engine#55001) 2024-09-06 [email protected] Change "there own" to "their own" in Flutter-GPU docs (flutter/engine#54921) 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
auto-submit bot
pushed a commit
that referenced
this pull request
Sep 9, 2024
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](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 #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 #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 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
jesswrd
pushed a commit
to jesswrd/engine
that referenced
this pull request
Sep 11, 2024
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
jesswrd
pushed a commit
to jesswrd/engine
that referenced
this pull request
Sep 11, 2024
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.