Repository navigation
Scenarios: Deduplicate scenario name declarations - #190818
Conversation
Previously, adding a scenario to the iOS scenario app meant registering it in three places: * the Dart factory in `scenarios.dart` * the launch argument in `SceneDelegate.m` * the golden identifier in `GoldenTestManager.m` The two Obj-C tables each had a comment asking whoever edits it to keep the other in sync. I missed one of the three adding scenarios recently, which is what prompted this cleanup. It seems better to enforce this through code than through comments. The mapping is pretty mechanical: drop the leading `--` and swap `-` for `_`. Both files now derive the name instead of looking it up. All 38 of `GoldenTestManager`'s entries were derivable that way, as were all but four of `SceneDelegate`'s 58. Those were the `--gesture-*` arguments, which dropped the `platform_view_` prefix that are included in the names of the scenarios they select. I've renamed those to `--platform-view-gesture-*` to match, which lets us delete those tables completely. `SceneDelegate` keeps a set of the arguments that select a scenario, since it still has to tell those apart from behaviour flags like `--screen-before-flutter` and can't look up from the Dart registry. The good news is these can no longer disagree, since neither of them stores any names. The gesture rename is safe since those arguments only occur in `PlatformViewGestureRecognizerTests.m`, and those tests assert on accessibility labels. They don't go through `GoldenTestManager` so there are no golden images named after them that need renaming. We now blow up on unrecognised `--` arguments rather than silently falling through to a bare `UIViewController`. As I (accidentally) discovered when trying to add a new platformview clip test, a scenario registered in Dart but missing from `SceneDelegate` used to show up thirty seconds later as a golden test timeout pointing at the platform view and the engine; it now complains about the argument and tells you where to register it. There's deliberately no allowlist of known non-scenario flags alongside that check. `--screen-before-flutter` has its own branch, and the two remaining behaviour flags, `--maskview-blocking` and `--with-continuous-texture`, only ever accompany a scenario argument, so we never reach the check while one is set. Passing either on its own now raises, which seems like the right outcome given it selects no scenario and would previously have left you looking at an empty view controller. Also discovered the `launchArgsMap` global from `GoldenTestManager` was dead code. It was exported from the header, then shadowed by a static local that held the real table, and was never assigned to or read.
There was a problem hiding this comment.
Code Review
This pull request refactors the iOS scenario app to dynamically derive Dart scenario names and golden identifiers from launch arguments by removing the leading '--' and replacing '-' with '_', eliminating the need for a static mapping dictionary. Additionally, test launch arguments are updated to use their full names. The review feedback suggests handling non-scenario arguments gracefully in SceneDelegate.m to prevent incorrect exceptions, and adding a defensive prefix check in GoldenTestManager.m to avoid potential out-of-bounds crashes on malformed arguments.
| for (NSString* argument in processArguments) { | ||
| if ([argument hasPrefix:@"--"]) { | ||
| [NSException raise:NSInvalidArgumentException | ||
| format:@"Unrecognised scenario argument \"%@\". Add it to scenarioArguments in " | ||
| @"SceneDelegate.m, and register the scenario in scenarios.dart.", | ||
| argument]; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current check for unrecognized arguments starting with -- will raise a misleading exception if behavior flags like --maskview-blocking or --with-continuous-texture are passed on their own (instructing the user to register them as scenarios in scenarios.dart). Additionally, it will crash on --app-extension which is a valid argument used in golden tests but not registered as a scenario in SceneDelegate.m.
We should handle these known non-scenario arguments gracefully by either raising a more specific exception or ignoring them.
for (NSString* argument in processArguments) {
if ([argument hasPrefix:@"--"]) {
if ([argument isEqualToString:@"--maskview-blocking"] ||
[argument isEqualToString:@"--with-continuous-texture"]) {
[NSException raise:NSInvalidArgumentException
format:@"Argument \"%@\" requires a scenario argument to be specified.",
argument];
} else if (![argument isEqualToString:@"--app-extension"]) {
[NSException raise:NSInvalidArgumentException
format:@"Unrecognised scenario argument \"%@\". Add it to scenarioArguments in "
@"SceneDelegate.m, and register the scenario in scenarios.dart.",
argument];
}
}
}There was a problem hiding this comment.
The current check for unrecognized arguments starting with
--will raise a misleading exception if behavior flags like--maskview-blockingor--with-continuous-textureare passed on their own (instructing the user to register them as scenarios inscenarios.dart).
Bad bot.
These flags exist and handled earlier in the same if/else chain. They are ONLY ever used in combination with an existing scenario test, so passing one of these on its own is an error, which will trigger the failure instead of just mysteriously hanging 30 seconds like it used to. That's a good thing.
Additionally, it will crash on
--app-extensionwhich is a valid argument used in golden tests but not registered as a scenario inSceneDelegate.m.
Bad bot.
This was deleted intentionally --app-extension isn't used by any Scenarios test, and it doesn't do anything.
This was listed as a key in GoldenTestManager's launchArgsMap in initWithLaunchArg:, but not a single one of its call sites passed in --app-extension. Grepping for app_extension shows that it was (and still is) hardcoded in the places it was used, because it has to be since it's launched in a separate, dedicated config/bundle.
Looking the ShareViewController code where we use it, you can see it doesn't take a launch arg at all: ShareViewController.m:26 hardcodes set_scenario with app_extension over the driver channel, which is how the app_extension scenario at scenarios.dart:155 actually runs. This isn't something we can really drive from the flag. There might be some way to drive this from a flag or a launch arg, but that's a separate refactoring.
For completeness... there's no golden_app_extension_* image in ScenariosUITests/ either. It turns out AppExtensionTests imported GoldenTestManager.h but never actually constructs one. It launches [[XCUIApplication alloc] init] with no launch arguments at all and verifies that a flutter_view element appears after tapping through the share sheet. In a way, I guess this was sort of useful feedback, cause I discovered we can delete the dead import.
For even more completeness... it's also not registered as a scenario in SceneDelegate.m, but it was also never in SceneDelegate's table before this change either.
TL;DR passing this flag to the app just fell through to a bare UIViewController. So the flag has never selected anything. We're going from "silently render a blank screen" to an error that complains about an invalid argument.
| _identifier = [[launchArg substringFromIndex:2] stringByReplacingOccurrencesOfString:@"-" | ||
| withString:@"_"]; |
There was a problem hiding this comment.
If launchArg does not start with -- or has a length of less than 2 characters, calling substringFromIndex:2 will throw an NSRangeException and crash the test runner. Adding a defensive check ensures that we handle unexpected or malformed launch arguments gracefully.
| _identifier = [[launchArg substringFromIndex:2] stringByReplacingOccurrencesOfString:@"-" | |
| withString:@"_"]; | |
| if ([launchArg hasPrefix:@"--"]) { | |
| _identifier = [[launchArg substringFromIndex:2] stringByReplacingOccurrencesOfString:@"-" | |
| withString:@"_"]; | |
| } else { | |
| _identifier = [launchArg stringByReplacingOccurrencesOfString:@"-" withString:@"_"]; | |
| } |
There was a problem hiding this comment.
Bad or at least super dubious bot.
launchArg isn't user input, it comes from initWithLaunchArg: call sites within this app and every single one of those is fine. If someone passes @"-" it should blow up and the user should fix it.
Previously `run_ios_tests.sh` only worked when invoked from `engine/src/flutter` which, to be fair, is what the READMEs tell you to do but there's no reason we need to force that, and we don't for other tests like run_tests.py. This is mostly just post-monorepo-merge cleanup. Two separate things depended on the working directory: * The wrapper was resolving `SCRIPT_DIR` from `BASH_SOURCE` but still passed the Dart entrypoint as a relative path, so the VM would report `No such file or directory`. * `run_ios_tests.dart` called `Engine.tryFindWithin()`, which defaults to the current directory and only walks upward. From the repo root `engine/src` is below the starting point, so the search fails and the script exits with `Must be run from within the engine repository.` Both now resolve from the script's own location, so the documented invocation keeps working and but invoking from any other directory works too. The Dart side matches the existing usage in `tools/header_guard_check/lib/header_guard_check.dart:162`. Also fixed up the READMEs, which had a few other issues... one if which was that I forgot to update them in flutter#190818. No test changes because this *is* test changes. <!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Previously, adding a scenario to the iOS scenario app meant registering it in three places:
scenarios.dartSceneDelegate.mGoldenTestManager.mThe two Obj-C tables each had a comment asking whoever edits it to keep the other in sync. I missed one of the three adding scenarios recently, which is what prompted this cleanup. It seems better to enforce this through code than through comments.
The mapping is pretty mechanical: drop the leading
--and swap-for_. Both files now derive the name instead of looking it up. All 38 ofGoldenTestManager's entries were derivable that way, as were all but four ofSceneDelegate's 58. Those were the--gesture-*arguments, which dropped theplatform_view_prefix that are included in the names of the scenarios they select. I've renamed those to--platform-view-gesture-*to match, which lets us delete those tables completely.SceneDelegatekeeps a set of the arguments that select a scenario, since it still has to tell those apart from behaviour flags like--screen-before-flutterand can't look up from the Dart registry. The good news is these can no longer disagree, since neither of them stores any names.The gesture rename is safe since those arguments only occur in
PlatformViewGestureRecognizerTests.m, and those tests assert on accessibility labels. They don't go throughGoldenTestManagerso there are no golden images named after them that need renaming.We now blow up on unrecognised
--arguments rather than silently falling through to a bareUIViewController. As I (accidentally) discovered when trying to add a new platformview clip test, a scenario registered in Dart but missing fromSceneDelegateused to show up thirty seconds later as a golden test timeout pointing at the platform view and the engine; it now complains about the argument and tells you where to register it.There's deliberately no allowlist of known non-scenario flags alongside that check.
--screen-before-flutterhas its own branch, and the two remaining behaviour flags,--maskview-blockingand--with-continuous-texture, only ever accompany a scenario argument, so we never reach the check while one is set. Passing either on its own now raises, which seems like the right outcome given it selects no scenario and would previously have left you looking at an empty view controller.Also discovered the
launchArgsMapglobal fromGoldenTestManagerwas dead code. It was exported from the header, then shadowed by a static local that held the real table, and was never assigned to or read.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.