Skip to content

[material_ui, cupertino_ui, infra] Support skipping golden files for the design migration#11649

Open
Piinks wants to merge 21 commits intoflutter:mainfrom
Piinks:goldSkipper
Open

[material_ui, cupertino_ui, infra] Support skipping golden files for the design migration#11649
Piinks wants to merge 21 commits intoflutter:mainfrom
Piinks:goldSkipper

Conversation

@Piinks
Copy link
Copy Markdown
Contributor

@Piinks Piinks commented May 5, 2026

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, matchesGoldenFile will 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: none here, 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

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-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.

Footnotes

  1. 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

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label May 5, 2026
@github-actions github-actions Bot added triage-framework Should be looked at in framework triage p: material_ui labels May 5, 2026
@Piinks Piinks changed the title Add goldctl dependency on unit tests [material_ui, cupertino_ui, infra] Support skipping golden files for the design migration May 5, 2026
@Piinks Piinks requested a review from justinmc May 5, 2026 14:30
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread script/flutter_goldens/lib/flutter_goldens.dart Outdated
Comment thread script/flutter_goldens/lib/flutter_goldens.dart Outdated
Comment thread script/flutter_goldens/pubspec.yaml
Comment thread packages/material_ui/test/goldens/goldens_test.dart Outdated
Comment thread script/flutter_goldens/lib/flutter_goldens.dart
Comment thread script/flutter_goldens/lib/flutter_goldens.dart Outdated
@github-actions github-actions Bot removed the CICD Run CI/CD label May 5, 2026
@Piinks Piinks added the CICD Run CI/CD label May 5, 2026
Copy link
Copy Markdown
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. After merging I can update my PR #11568 and then make sure it correctly skips the golden tests.

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call for now. I'll plan to remove this when we are ready to attempt to publish the first pre-release version.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks. Let me double check with Stuart before merging this.

@Piinks Piinks requested a review from stuartmorgan-g May 5, 2026 18:26
@github-actions github-actions Bot removed the CICD Run CI/CD label May 5, 2026
@Piinks Piinks added the CICD Run CI/CD label May 5, 2026
path: any
platform: any
process: any
yaml: any
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are all of these any? That's almost guaranteed to cause breakage in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the same as in flutter/flutter, and AFAIK it has not caused breakages. What should it be? The latest?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the same as in flutter/flutter, and AFAIK it has not caused breakages.

flutter/flutter uses a repo-level workspace and checks in a lock file for it, so that every version of everything is exactly controlled in CI. This repo just uses standard pub resolution on a per-package basis, and (per Dart standards) doesn't check in lock files for packages.

So any in flutter/flutter is effectively "don't get in the way of the script that updates the lock file, which is where everything is actually controlled", whereas any here has the typical pub meaning of "Give me literally any version (which will in practice be the latest resolvable version every time it runs), and I don't care if it's a major version with breaking changes".

What should it be? The latest?

Typically we want packages depending on a ^ version for the latest major version, but I'm not sure what exactly this particular package needs. If you want to mostly match flutter/flutter's current behavior you could use a ^ range starting with whatever the flutter/flutter pubspec.lock version is for each package. If you want to exactly match flutter/flutter you could exactly pin every version here, but since we don't have a pub dependency roller for this repo that would mean a lot more manual work managing dependency versions over time.

Comment thread packages/cupertino_ui/test/goldens/goldens_test.dart
publish_to: none

environment:
sdk: ^3.9.0-0
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this using a prerelease version format?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably because I copied most of this over from flutter/flutter for consistency. 😅
Let me fix this, thank you!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, unless it's the *-0?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, X-0 means "any pre-release version of X". I would not expect us to need our tooling to support old prereleases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD p: material_ui triage-framework Should be looked at in framework triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants