test(goldens): render goldens with the real app theme - #521
Conversation
Every golden test built its own bare ThemeData(colorScheme: ...) instead of calling buildAppTheme(), so nothing in the real theme beyond the colour scheme was covered: appBarTheme, navigationBarTheme and the whole text theme were invisible to all 15 goldens. That gap hid a real WCAG failure in the MyFestivalScreen app bar, which was visible by eye on a deployed preview but not to a golden of that exact screen. The blocker was google_fonts fetching over the network under test. It turned out the suite was already fetching fonts from fonts.gstatic.com on every run via the tests that do use buildAppTheme, so the goldens would have inherited a hidden dependency on CDN reachability. Bundle the six variants buildAppTextTheme actually asks for in assets/fonts/, and turn runtime fetching off suite-wide in a new test/flutter_test_config.dart. google_fonts checks the asset bundle before the network, so the typefaces now resolve locally: goldens are deterministic on any machine, and a weight added without a matching file fails loudly instead of silently depending on the network. This also removes a network round-trip from first paint in the shipped app, which matters on festival wifi. All 15 goldens are regenerated and reviewed. They previously rendered every glyph as the blocky FlutterTest placeholder box, so they could not catch a typography or text-layout regression at all; they now show real text in the real faces. Fixes #520
There was a problem hiding this comment.
Pull request overview
This PR makes golden/screenshot tests render with the real app theme (buildAppTheme) and makes google_fonts deterministic under test by disabling runtime fetching and bundling the required font variants as assets. This closes the gap where theme regressions (app bar, nav bar, typography) were invisible to goldens due to ad-hoc ThemeData usage and placeholder glyph rendering.
Changes:
- Update golden-producing tests to use
buildAppTheme(brightness)instead of locally-constructedThemeData. - Add
test/flutter_test_config.dartto disableGoogleFonts.config.allowRuntimeFetchingsuite-wide so tests rely on bundled font assets. - Document the bundled-font approach and add the asset entry for
assets/fonts/.
Reviewed changes
Copilot reviewed 9 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/widgets/drink_card_test.dart | Switches test MaterialApp theme to buildAppTheme so goldens cover real text/theme styling. |
| test/style_screen_screenshot_test.dart | Uses buildAppTheme for both light/dark screenshot tests to cover app bar/nav/text theme. |
| test/screens/my_festival_screen_test.dart | Removes custom golden theme and uses buildAppTheme for light/dark goldens. |
| test/flutter_test_config.dart | Disables Google Fonts runtime fetching for deterministic, offline-safe tests. |
| test/drink_detail_screen_screenshot_test.dart | Uses buildAppTheme for screenshot tests instead of inline ThemeData. |
| test/brewery_screen_screenshot_test.dart | Uses buildAppTheme for screenshot tests instead of inline ThemeData. |
| pubspec.yaml | Adds assets/fonts/ asset entry with rationale for bundling fonts for tests and first paint. |
| docs/README.md | Links the new fonts documentation. |
| docs/code/fonts.md | Documents why fonts are bundled and how to add additional weights/variants safely. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-520-goldens-real-theme.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
Bundling the font binaries changed the app from linking to Google's CDN to redistributing the files, and both families are SIL Open Font License 1.1, which requires the licence to ship alongside them. Nothing in lib/ registered anything with LicenseRegistry. Add the two OFL texts to assets/fonts/ and register them from main() before runApp, so they appear in the standard "View licences" page. loadFontLicenses() is split out from registerFontLicenses() so tests can drain the collector directly — LicenseRegistry.licenses never completes under flutter_test. The tests load the real assets, so a wrong path fails rather than silently registering an empty licence. They read through tester.runAsync because a rootBundle load never completes inside testWidgets' FakeAsync zone; without it the test hangs until the 10-minute timeout instead of failing. Also correct two review comments: testExecutable wraps each test file's main() rather than running before every test case, and adding a font weight needs no pubspec edit because assets/fonts/ is declared as a directory. Note in web/_headers that the Google Fonts CSP origins are now only a fallback; the policy itself is left alone.
|
Pushed Licensing — the substantive one. Bundling the font binaries changed this app from linking to Google's CDN into redistributing the files. Both families are SIL Open Font License 1.1, which requires the licence to ship alongside them, and nothing in Now: Both review comments were correct and are fixed.
One trap worth recording, since it cost two wrong diagnoses before I isolated it with a probe: a Left deliberately alone:
Generated by Claude Code |
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-520-goldens-real-theme.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
codecov/patch failed at 66.66% because registerFontLicenses() was never executed by a test — the licence-loading stream was covered but the one-line glue that hands it to the LicenseRegistry was not. Drain the registry itself after resetting Flutter's own collectors, so the wiring is verified end to end rather than the function merely being called. The drain runs inside tester.runAsync: testWidgets' fake-async zone never advances the real asset read the collector awaits, which is why an earlier attempt hung to the 10-minute test timeout instead of failing.
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-520-goldens-real-theme.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
Fixes #520
What was wrong
Every golden test built its own bare
ThemeData(colorScheme: ColorScheme.fromSeed(...))instead of callingbuildAppTheme(). Nothing in the real theme beyond the colour scheme was covered —appBarTheme,navigationBarThemeand the entire text theme were invisible to all 15 goldens. That is the gap that let a real WCAG failure survive in theMyFestivalScreenapp bar: visible by eye on a deployed preview, invisible to a golden of that exact screen in both themes.What the investigation turned up
The stated blocker was that
google_fontsfetches over the network under test. Two findings changed the shape of the fix:fonts.gstatic.comis reachable from CI-like environments, and the tests that already usebuildAppTheme(overflow_menu_test,widgets_test,festival_menu_sheets_test,my_festival_screen_test) were quietly downloading typefaces mid-test. Pointing the goldens at the real theme would have turned that hidden dependency into a pixel dependency on CDN reachability.allowRuntimeFetching = falsedoes not fall back gracefully —google_fonts_base.dart:173rethrows, so the test fails outright. Disabling fetching alone was not an option.The fix
Bundle the six variants
buildAppTextThemeactually asks for inassets/fonts/, and turn runtime fetching off suite-wide via a newtest/flutter_test_config.dart.google_fontschecks the asset bundle before the network, so the typefaces resolve locally.buildAppTextThemewithout a matching file fails the suite loudly rather than silently depending on a network fetch that only works on a connected machine.lib/main.dartalready carries anisTransientFontLoadErrorhelper to downgrade google_fonts fetch failures in Crashlytics; this removes the cause rather than filtering the symptom.The five golden-producing files now call
buildAppTheme(brightness);_goldenThemeinmy_festival_screen_test.dartis gone along with its now-incorrect comment.Licensing
Bundling changes the app from linking to Google's CDN to redistributing the font binaries, and both families are SIL OFL 1.1, which requires the licence to ship with the files. There was no
LicenseRegistrycall anywhere inlib/before this.assets/fonts/now also holdsOFL-NunitoSans.txtandOFL-PlayfairDisplay.txt, andregisterFontLicenses()inlib/app_theme.dart(called frommain()beforerunApp) adds them to Flutter'sLicenseRegistryso they appear in the standard "View licences" page.About the regenerated goldens
All 15 are regenerated. The visual change is large and deliberate: they previously rendered every glyph as the blocky
FlutterTestplaceholder box, so no golden could catch a typography or text-layout regression at all. They now show real text in Playfair Display and Nunito Sans. Each was reviewed by eye before committing.Verification
./bin/mise run check— clean, 1309 tests passtestjob passes against them, which is the real proof they render identically off this machinebuild:webandbuild-androidboth succeed; the six.ttffiles ship inbuild/web/assets/assets/fonts/google_fontsmanifest by SHA-256 and byte length before being committedtest/app_theme_test.dart, so a wrong path fails the suite instead of silently registering nothingEvidence that fonts resolve from assets rather than the network:
google_fontschecks the asset bundle before attempting HTTP, and the suite passes withallowRuntimeFetching = false, which only succeeds if all six variants resolve locally. Note thatsmoke-test-previewpassing is not evidence of this — the CSP still permitsfonts.gstatic.com, so a fetch would not raise a violation either way. It shows only that there is no CSP regression.Scope
test/— five golden files switched to the real theme, newtest/flutter_test_config.dart, licence testslib/app_theme.dart,lib/main.dart— licence registration only; no theme or behaviour changepubspec.yaml— one asset directory entry (~674 KB of fonts)web/_headers— comment only, no policy change. Thefonts.gstatic.comentries are now a fallback rather than load-bearing; dropping them is a security decision left to the maintainer.docs/code/fonts.md, linked fromdocs/README.mdNot covered
Material icon glyphs still render as hollow boxes in goldens —
MaterialIconsis not loaded byflutter_test. Pre-existing and unchanged by this PR; goldens cover icon position and size, not the glyph. Noted indocs/code/fonts.md.Manual browser/device confirmation of the bundled fonts in a real app run has not been done — it cannot be performed by an agent and remains outstanding.