[ios]do not nuke user input path when running uiscene integration test#186436
[ios]do not nuke user input path when running uiscene integration test#186436hellohuanlin wants to merge 1 commit into
Conversation
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.
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.