[ios]do not nuke user input path when running uiscene integration test - #186436
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the iOS UI scene test task to create the generated project within a subfolder named 'flutter_uiscene_test_generated_project' instead of the root destination directory. Feedback includes a likely missing import for the 'path' package which will cause a compilation error, potential regressions in subsequent logic or external tools that rely on the original output path, and the need to update documentation to reflect the new directory structure.
| if (destination != null) { | ||
| destinationOverride = true; | ||
| destinationDir = Directory(destination); | ||
| destinationDir = Directory(path.join(destination, 'flutter_uiscene_test_generated_project')); |
There was a problem hiding this comment.
The code uses path.join, which requires the package:path/path.dart library to be imported (typically as path). Since the previous version of this code used the Directory constructor directly with the destination string, this import is likely missing. This will cause a compilation error when the script is run, especially given the note in the PR description that the changes have not been tested locally.
| if (destination != null) { | ||
| destinationOverride = true; | ||
| destinationDir = Directory(destination); | ||
| destinationDir = Directory(path.join(destination, 'flutter_uiscene_test_generated_project')); |
There was a problem hiding this comment.
Moving the generated project into a subfolder (flutter_uiscene_test_generated_project) changes the output location. It is critical to ensure that all subsequent logic in this script, as well as any external tools or CI configurations that rely on this task, are updated to look for the project at destinationDir.path rather than the original destination path. If any code still uses the destination variable to construct paths to project files (e.g., to find the .xcodeproj), those references will now be broken.
| if (destination != null) { | ||
| destinationOverride = true; | ||
| destinationDir = Directory(destination); | ||
| destinationDir = Directory(path.join(destination, 'flutter_uiscene_test_generated_project')); |
There was a problem hiding this comment.
The PR description mentions that the documentation is being kept untouched. However, if the documentation for the --destination parameter or the script's usage instructions states that the project is created at the provided path, it is now inaccurate. The documentation should be updated to reflect that a subfolder is created to prevent accidental data loss in the user-provided directory.
There was a problem hiding this comment.
wrong, the instruction specifically says will save to the destination, not will replace the destination.
|
Greetings from stale PR triage! 👋 |
hellohuanlin
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
flutter/flutter@9988960...0f02463 2026-07-28 [email protected] Roll Fuchsia Test Scripts from E8hJ1AfK8CtGtaES0... to 1frGe_KltAJKkeyPg... (flutter/flutter#190134) 2026-07-28 [email protected] iOS: Reject merged-platform-ui-thread=mergeAfterLaunch (flutter/flutter#190051) 2026-07-28 [email protected] iOS: Migrate TaskRunner tests to Swift Testing (flutter/flutter#190055) 2026-07-28 [email protected] Run Mac golden tests on ARM bots (flutter/flutter#189465) 2026-07-28 [email protected] iOS,macOS: Rename Swift test files to end in Tests.swift (flutter/flutter#190063) 2026-07-28 [email protected] Fix hcpp cliprect being behind by 1 frame when scrolling (flutter/flutter#189946) 2026-07-28 [email protected] Roll Fuchsia Linux SDK from vpboK5fPPIoFteqRq... to OZkZC_2CZ_G5rbMIS... (flutter/flutter#190115) 2026-07-27 [email protected] Add Ishaq Hassan to AUTHORS (flutter/flutter#190064) 2026-07-27 [email protected] [wimp] fixes ubo padding size issue (flutter/flutter#189958) 2026-07-27 [email protected] Roll pub packages (flutter/flutter#189872) 2026-07-27 [email protected] Move tool host_cross_arch tests into different shards (flutter/flutter#189470) 2026-07-27 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 6.2.0 to 7.0.0 in the all-github-actions group (flutter/flutter#190099) 2026-07-27 [email protected] [ios]do not nuke user input path when running uiscene integration test (flutter/flutter#186436) 2026-07-27 [email protected] ci: verify_binaries_pre_codesigned part 2 (flutter/flutter#190078) 2026-07-27 [email protected] Roll Abseil to ff6e8ce3e932 (flutter/flutter#189998) 2026-07-27 [email protected] Android_hardware_smoke_test: clean up golden copy in CI (flutter/flutter#189948) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
4 participants
As discussed in #186412 (comment).
Note:
The documentation itself is fine tho, so I will keep it untouched.
It looks like we don't have infra setup to write unit tests for test scripts, so I will leave it as out-of-scope of this PR.
But in the future we should be heading towards dumb logic in test scripts, and leverage Xcode build targets & file memberships instead.
I think this is a broader security issue than just this particular script - I wish our flutter/dart tooling could have asked for permission for dangerous operations like this. I filed [flutter/dart] Is it possible to ask for permission for dangerous operations for Flutter contributors #186419
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
Fixes #186412
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
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.