Redirect `run_ios_tests.sh` to `run_ios_tests.dart`. #53717

matanlurey · 2024-07-03T19:07:19Z

If this merges cleanly, and is not reverted, we can delete run_ios_tests.sh and be done.

jmagman · 2024-07-10T23:23:55Z

testing/scenario_app/bin/run_ios_tests.dart

    ],
  );
  if (result.exitCode != 0) {
    throw Exception('Failed to zip the test results: ${result.stderr}');
  }
+  resultBundle.deleteSync(recursive: true);


@matanlurey taking a crack at fixing this 99a2e4e -- I think the difference is that the results aren't deleted between runs.

Close, it was the dumpDir above leaking. PTAL!

jmagman · 2024-07-11T22:41:07Z

testing/scenario_app/bin/run_ios_tests.dart

-    dumpXcresultOnFailurePath = io.Directory.systemTemp.createTempSync().path;
+    final dumpDir = io.Directory.systemTemp.createTempSync();
+    dumpXcresultOnFailurePath = dumpDir.path;
+    cleanup.add(() => dumpDir.delete(recursive: true));


@matanlurey it needs to be deleted between the Skia and Impeller run though, not at the end of running both.

Ah yes, you are right. Thank you!

jmagman

LGTM when it passes, thank you so much for working on this!

zanderso · 2024-07-18T20:08:11Z

From PR triage: @matanlurey, is this still on your radar?

matanlurey · 2024-07-18T20:36:31Z

Yeah. @jmagman was messing around to see if she understood the last failure.

I'll take it back over and get this landed.

jmagman · 2024-07-18T23:20:09Z

@matanlurey I got it past the bundle already existing, and now the tests are actually failing because of some simulator issue: #53965

I haven't had time to look into it again though.

zanderso · 2024-07-25T20:06:08Z

From PR triage: I am going to mark this one WIP.

flutter-dashboard · 2024-08-21T23:52:42Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

jmagman · 2024-09-07T04:01:08Z

So I think the shell script wasn't doing what anyone thought, and by moving it to dart you "fixed" the parameters and now it doesn't run. I'm trying to fix just the shell script and am hitting the same "Failed to set placeholder attributes dev.flutter.Scenarios.ScenariosShare" error: #55012.

Once I fix it there, I'll update the dart script to match and then I bet it will work.

matanlurey · 2024-09-09T20:26:05Z

So I think the shell script wasn't doing what anyone thought, and by moving it to dart you "fixed" the parameters and now it doesn't run. I'm trying to fix just the shell script and am hitting the same "Failed to set placeholder attributes dev.flutter.Scenarios.ScenariosShare" error: #55012.

Once I fix it there, I'll update the dart script to match and then I bet it will work.

Thanks for looking! I admit I wasn't sure how to proceed and was racking my brain trying to understand how I introduced an accidental breaking change of some kind, so that makes me feel better.

Let me know if I can help somehow.

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

jmagman · 2024-09-09T22:41:09Z

#55012 merged, I rebased onto it, and fixed quotes in the dart script that were incorrect.

jmagman · 2024-09-09T23:34:27Z

The actual tests are now passing, but the script is failing:

Unhandled exception:
PathNotFoundException: Deletion failed, path = '/Volumes/Work/s/w/ir/cache/builder/src/out/ci/ios_debug_unopt_sim/scenario_app/Scenarios/ios_scenario_xcresultIT41TV' (OS Error: No such file or directory, errno = 2)
#0      _Directory._deleteSync (dart:io/directory_impl.dart:188:7)

matanlurey · 2024-09-10T01:44:15Z

I'll take a look tomorrow!

…sh-redirect

jmagman

Thank you so so much for sticking with this!

matanlurey · 2024-09-10T21:03:54Z

Thank you so so much for sticking with this!

And thank you for helping to debug it!

…er/engine#53717)

…154960) flutter/engine@8a038a6...60c15bc 2024-09-10 [email protected] Update package:web to use new path and latest revision (flutter/engine#55085) 2024-09-10 [email protected] Roll Skia from 82d2e4033333 to 1760de26fb83 (8 revisions) (flutter/engine#55084) 2024-09-10 [email protected] Move more of the tests in the engine to `package:test` (flutter/engine#55083) 2024-09-10 [email protected] Redirect `run_ios_tests.sh` to `run_ios_tests.dart`. (flutter/engine#53717) 2024-09-10 [email protected] [Impeller] reland experimental canvas. (flutter/engine#55047) 2024-09-10 [email protected] Fix 'dart --enable-asserts' typo in license check script (flutter/engine#55082) 2024-09-10 [email protected] Roll Dart SDK from af7460619b2b to 741512e72e92 (1 revision) (flutter/engine#55080) 2024-09-10 [email protected] Roll Skia from 4815ec808aa5 to 82d2e4033333 (8 revisions) (flutter/engine#55079) 2024-09-10 [email protected] Roll buildroot to f85c3be4bf808add6ba867b8ff7943fd235b7b5e. (flutter/engine#55051) 2024-09-10 [email protected] Roll Skia from 5dc067350ab5 to 4815ec808aa5 (2 revisions) (flutter/engine#55074) 2024-09-10 [email protected] Roll Dart SDK from d59138eb6ec2 to af7460619b2b (1 revision) (flutter/engine#55073) 2024-09-10 [email protected] Roll Skia from c62380453c72 to 5dc067350ab5 (1 revision) (flutter/engine#55072) 2024-09-10 [email protected] Roll Fuchsia Linux SDK from kIWQ460iagZLxVRPc... to 8XJTJm4D4Go1dBaTi... (flutter/engine#55071) 2024-09-10 [email protected] Roll Skia from 9bea95918e69 to c62380453c72 (1 revision) (flutter/engine#55070) 2024-09-10 [email protected] Roll Dart SDK from a60078a23978 to d59138eb6ec2 (2 revisions) (flutter/engine#55068) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from kIWQ460iagZL to 8XJTJm4D4Go1 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

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

Part of flutter/flutter#143953. If this merges cleanly, and is not reverted, we can delete `run_ios_tests.sh` and be done.

matanlurey force-pushed the run-ios-tests-dart-sh-redirect branch from 956f583 to cf0bd52 Compare July 3, 2024 19:21

jmagman force-pushed the run-ios-tests-dart-sh-redirect branch from ebf848d to 99a2e4e Compare July 10, 2024 23:23

jmagman reviewed Jul 10, 2024

View reviewed changes

jmagman reviewed Jul 11, 2024

View reviewed changes

matanlurey marked this pull request as ready for review July 11, 2024 22:46

jmagman approved these changes Jul 11, 2024

View reviewed changes

jmagman mentioned this pull request Jul 17, 2024

Scenario dart script #53965

Closed

8 tasks

zanderso added the Work in progress (WIP) Not ready (yet) for review! label Jul 25, 2024

matanlurey and others added 8 commits September 9, 2024 15:38

Redirect run_ios_tests.sh to run_ios_tests.dart.

401c5ff

Delete bundle between runs

3e3816b

++

efb54b5

++

eccb6db

++

26f61d1

++

d6f2d36

++

ab8b35f

Fix quotes

3a9683c

jmagman force-pushed the run-ios-tests-dart-sh-redirect branch from 50b8c00 to 3a9683c Compare September 9, 2024 22:40

matanlurey added 2 commits September 10, 2024 11:20

Merge remote-tracking branch 'upstream/main' into run-ios-tests-dart-…

b09eab5

…sh-redirect

Move resultBundle.delete into the if block.

df1255e

matanlurey removed the Work in progress (WIP) Not ready (yet) for review! label Sep 10, 2024

Conditonally delete.

0078d9b

matanlurey requested a review from jmagman September 10, 2024 20:42

jmagman approved these changes Sep 10, 2024

View reviewed changes

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 10, 2024

auto-submit bot merged commit 5882da8 into flutter:main Sep 10, 2024
32 checks passed

jmagman mentioned this pull request Sep 10, 2024

Scenario App Integration Tests are not uploading Xcode xcresults on failure flutter/flutter#154956

Closed

matanlurey mentioned this pull request Sep 10, 2024

Converting engine scenario run scripts to python or dart flutter/flutter#143953

Closed

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

Roll Flutter Engine from 8a038a6f7099 to 60c15bc0f40e (15 revisions) flutter/flutter#154960

Merged

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

5882da8e2 Redirect run_ios_tests.sh to run_ios_tests.dart. (flutt…

bd697a3

…er/engine#53717)

jmagman mentioned this pull request Sep 11, 2024

Uploaded engine iOS scenario test Xcode xcresults unzip with full absolute paths flutter/flutter#155043

Open

jesswrd pushed a commit to jesswrd/engine that referenced this pull request Sep 11, 2024

Redirect run_ios_tests.sh to run_ios_tests.dart. (flutter#53717)

5aba8ce

Part of flutter/flutter#143953. If this merges cleanly, and is not reverted, we can delete `run_ios_tests.sh` and be done.

Redirect run_ios_tests.sh to run_ios_tests.dart. #53717

Redirect run_ios_tests.sh to run_ios_tests.dart. #53717

Uh oh!

Conversation

matanlurey commented Jul 3, 2024

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matanlurey Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

jmagman Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

matanlurey Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso commented Jul 18, 2024

Uh oh!

matanlurey commented Jul 18, 2024

Uh oh!

jmagman commented Jul 18, 2024

Uh oh!

zanderso commented Jul 25, 2024

Uh oh!

flutter-dashboard bot commented Aug 21, 2024

Uh oh!

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

Uh oh!

Uh oh!

matanlurey commented Sep 9, 2024

Uh oh!

jmagman commented Sep 9, 2024

Uh oh!

jmagman commented Sep 9, 2024

Uh oh!

matanlurey commented Sep 10, 2024

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

matanlurey commented Sep 10, 2024

Uh oh!

Uh oh!

Uh oh!

Redirect `run_ios_tests.sh` to `run_ios_tests.dart`. #53717

Redirect `run_ios_tests.sh` to `run_ios_tests.dart`. #53717

jmagman Jul 10, 2024 •

edited

Loading

jmagman commented Sep 7, 2024 •

edited

Loading