[material_ui, cupertino_ui, infra] Support skipping golden files for the design migration#11649
[material_ui, cupertino_ui, infra] Support skipping golden files for the design migration#11649Piinks wants to merge 21 commits intoflutter:mainfrom
Conversation
github-actions
Bot
added
triage-framework
Piinks
changed the title
![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 introduces a new internal package, flutter_goldens, to support Skia Gold integration for golden image testing. It includes CI script updates, configuration for cupertino_ui and material_ui packages, and the core implementation of custom golden file comparators. Feedback highlights the need for proper URI-to-path conversion using the path package, correcting the Dart SDK version, fixing an incorrect import in material_ui, and ensuring safer YAML parsing.
| description: The official Flutter Material UI Library, implementing Google's Material Design design system. | ||
| version: 0.0.1 | ||
| version: 0.0.2 | ||
| publish_to: none |
There was a problem hiding this comment.
Good call for now. I'll plan to remove this when we are ready to attempt to publish the first pre-release version.
There was a problem hiding this comment.
Ok thanks. Let me double check with Stuart before merging this.
| path: any | ||
| platform: any | ||
| process: any | ||
| yaml: any |
There was a problem hiding this comment.
Why are all of these any? That's almost guaranteed to cause breakage in the future.
There was a problem hiding this comment.
So this is the same as in flutter/flutter, and AFAIK it has not caused breakages. What should it be? The latest?
There was a problem hiding this comment.
Are any of these file paths that exist in the real packages? Collisions are likely to make the import more difficult, so ideally any placeholders should use different names than real files.
There was a problem hiding this comment.
Yup, that is intentional. These are not existing file paths so they should not create collisions so that it is easier to merge with the existing mega-migration PR.
| publish_to: none | ||
|
|
||
| environment: | ||
| sdk: ^3.9.0-0 |
There was a problem hiding this comment.
Why is this using a prerelease version format?
There was a problem hiding this comment.
Probably because I copied most of this over from flutter/flutter for consistency. 😅
Let me fix this, thank you!
There was a problem hiding this comment.
Wait, yeah I thought I had checked this.. other packages here use the same format:
What should it be? I can send a separate PR to fix others if they are wrong too.
There was a problem hiding this comment.
Oh, unless it's the *-0?
3 participants
This branches off of #10753 and strips it down to just the skipping comparator. This means when golden file testing is used with this version of flutter_goldens,
matchesGoldenFilewill return true in all environments without executing image comparison.The goal of this PR is to join the material_ui and cupertino_ui mega-migration PR (cc @justinmc ) to disable golden file testing for now in material_ui and cupertino_ui so we can get a clearer signal from CI about any remaining blockers.
Full golden file testing support will likely land in #10753 as a separate step given the enormity of these PRs.
cupertino_ui and material_ui are set
publish_to: nonehere, which is intentional. This allows us to split up these changes, and wait to publish until everything lands and is working as we want it to.Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
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.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2