Skip to content

Reland "[data_assets] Cleanup tests" - #184714

Merged
auto-submit[bot] merged 2 commits into
flutter:masterfrom
dcharkes:reland-data-asset-cleanup
Apr 7, 2026
Merged

auto-submit[bot] merged 2 commits into
flutter:masterfrom
dcharkes:reland-data-asset-cleanup

Conversation

@dcharkes

@dcharkes dcharkes commented Apr 7, 2026 •

Copy link
Copy Markdown
Contributor

Relands #184209 adding a mac shard as suggested in #184209 (comment).

For adding a shard, I followed #154444, which suggests it's contained in the single .ci.yaml file, and the new shard names need to have bringup: true.


Original PR description:

This cleans up the data_assets experiment testing.

  • Use integration test projects, which are statically analyzed, formatted, etc.
  • This forces us to be explicit about the package:data_assets dependency.
  • This means the test projects will roll their dependencies with the dependency auto roller. (The test project was still using native_assets_cli which has been replaced with hooks and data_assets.)
  • Pin package:data_assets, because it's experimental, and we might do breaking changes.

Also makes the test projects more useful:

  • The data_assets_app now displays the assets in the UI. (Not only prints to the console for the integration tests.)
  • The data_assets_package now actually uses its asset.

And cleans up the test projects:

  • Move the test files into the data/ dir.
  • Give the test data assets a proper extension.

I saw weird behavior on Windows:

  • DevFS doesn't pick up the writes to the Dart files, not even with flushing or sleeps: Compiling dart to kernel with 0 updated files leading to a state where the Dart variable contents don't get updated while the data assets do. This integration test is not really about Dart hot restart, so I've skipped those checks on Windows.
  • This reproduces locally in the integration test, but not when running the app and editing the file.

The checks in dev/bots/analyze.dart also required some updates:

  • The template files check didn't allow for the HTML license.

P.S. Sorry for all the generated Flutter scaffolding. It seems that that is the way it's done in the integration test directory. (The package with the data assets is clean, but the Flutter app ...)

@github-actions github-actions Bot added tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels Apr 7, 2026
@dcharkes dcharkes added CICD Run CI/CD and removed tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop CICD Run CI/CD labels Apr 7, 2026
@github-actions github-actions Bot added tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels Apr 7, 2026
@dcharkes dcharkes added the CICD Run CI/CD label Apr 7, 2026
@dcharkes
dcharkes force-pushed the reland-data-asset-cleanup branch from 349f0c3 to 18a26b6 Compare April 7, 2026 07:56
@github-actions github-actions Bot removed the CICD Run CI/CD label Apr 7, 2026
@dcharkes dcharkes added the CICD Run CI/CD label Apr 7, 2026
@dcharkes
dcharkes marked this pull request as ready for review April 7, 2026 07:58
@dcharkes
dcharkes requested a review from goderbauer April 7, 2026 07:58

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

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.

Code Review

This pull request introduces a comprehensive integration test suite for Dart data assets, including a new test application and a dependency package. It updates CI targets to incorporate these tests, pins the data_assets package version, and refactors license verification logic to handle HTML formats. Feedback suggests enhancing error messages during asset loading, implementing concurrency guards for asynchronous timers in the test application, and adopting more robust regular expressions for string replacements in test scripts.

try {
found[assetId] = await rootBundle.loadString(assetId);
} catch (e) {
print('EXCEPTION $e');

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.

medium

The error message EXCEPTION $e is not very descriptive. Including the assetId that failed to load would make debugging much easier if the integration test fails.

Suggested change
print('EXCEPTION $e');
print('EXCEPTION loading $assetId: $e');

Comment on lines +32 to +55
_timer = Timer.periodic(const Duration(milliseconds: 500), (_) async {
final AssetData? appAssetData = await loadAppAssets();
final String dependencyAssetContent = await data_asset_package.loadAssetContent();
// Print for integration test discovery
if (appAssetData != null) {
print('VERSION: ${appAssetData.version}');
for (final MapEntry(:key, :value) in appAssetData.found.entries) {
print('FOUND "$key": "$value".');
}
for (final String id in appAssetData.notFound) {
print('NOT_FOUND "$id".');
}
}
print('DEPENDENCY_ASSET: $dependencyAssetContent');

if (mounted) {
setState(() {
if (appAssetData != null) {
_assetData = appAssetData;
}
_dependencyAsset = dependencyAssetContent;
});
}
});

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.

medium

Using Timer.periodic with an async callback can lead to overlapping executions if the asynchronous operations take longer than the timer interval. This can cause race conditions or noisy output in integration tests. Consider using a flag to prevent concurrent executions.

    bool isProcessing = false;
    _timer = Timer.periodic(const Duration(milliseconds: 500), (_) async {
      if (isProcessing) {
        return;
      }
      isProcessing = true;
      try {
        final AssetData? appAssetData = await loadAppAssets();
        final String dependencyAssetContent = await data_asset_package.loadAssetContent();
        // Print for integration test discovery
        if (appAssetData != null) {
          print('VERSION: ${appAssetData.version}');
          for (final MapEntry(:key, :value) in appAssetData.found.entries) {
            print('FOUND "$key": "$value".');
          }
          for (final String id in appAssetData.notFound) {
            print('NOT_FOUND "$id".');
          }
        }
        print('DEPENDENCY_ASSET: $dependencyAssetContent');

        if (mounted) {
          setState(() {
            if (appAssetData != null) {
              _assetData = appAssetData;
            }
            _dependencyAsset = dependencyAssetContent;
          });
        }
      } finally {
        isProcessing = false;
      }
    });

entity.copySync(targetFile.path);
if (relativePath == 'pubspec.yaml') {
String content = targetFile.readAsStringSync();
content = content.replaceFirst('resolution: workspace', '');

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.

medium

The string replacement replaceFirst('resolution: workspace', '') is fragile as it depends on exact spacing and doesn't handle potential comments or trailing whitespace on the same line. Consider using a regular expression to match the line more robustly.

Suggested change
content = content.replaceFirst('resolution: workspace', '');
content = content.replaceFirst(RegExp(r'^\s*resolution:\s*workspace\s*$', multiLine: true), '');

@flutter-dashboard

Copy link
Copy Markdown

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #184714 at sha 18a26b6

@flutter-dashboard flutter-dashboard Bot added the will affect goldens Changes to golden files label Apr 7, 2026

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@dcharkes dcharkes added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 7, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Apr 7, 2026
Merged via the queue into flutter:master with commit a0924c7 Apr 7, 2026
153 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 7, 2026
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 7, 2026
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 7, 2026
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 7, 2026
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 7, 2026
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Apr 7, 2026
flutter/flutter@9cd60b5...a0924c7

2026-04-07 [email protected] Reland "[data_assets] Cleanup tests" (flutter/flutter#184714)
2026-04-07 [email protected] Use the WindowRegistry in the multiple_windows example app (flutter/flutter#184579)
2026-04-07 [email protected] Introduce command to build a swift package for SwiftPM add to app integration (flutter/flutter#184660)
2026-04-07 [email protected] Have `flutter create` create a pubspec.lock to ensure pinned versions are being used. (flutter/flutter#175352)
2026-04-07 [email protected] [widgets/raw_menu_anchor.dart] Always call onClose and onCloseRequested on descendants before parent. (flutter/flutter#182357)
2026-04-07 [email protected] `WindowsPlugin` should not crash when ffiPlugin enabled (flutter/flutter#184695)
2026-04-06 [email protected] Use full goto.google.com hostname for go/ links (flutter/flutter#184679)
2026-04-06 [email protected] Apply rect clipping to surface views (flutter/flutter#184471)
2026-04-06 [email protected] [A11y] Allow percentage strings like "50%" as `SemanticsValue` for `ProgressIndicator` (flutter/flutter#183670)
2026-04-06 [email protected] Fix invisible accessibility element before scroll view (flutter/flutter#184155)
2026-04-06 [email protected] Roll Skia from 163dfdf500c7 to e264d870a380 (2 revisions) (flutter/flutter#184674)
2026-04-06 [email protected] Keep last character obscured when toggling obscureText (flutter/flutter#183488)
2026-04-06 [email protected] Parse scheme file with XML parser for SwiftPM migrator (flutter/flutter#184525)

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],[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
mbcorona pushed a commit to mbcorona/flutter that referenced this pull request Apr 15, 2026
Relands flutter#184209 adding a mac
shard as suggested in
flutter#184209 (comment).

For adding a shard, I followed
flutter#154444, which suggests it's
contained in the single `.ci.yaml` file, and the new shard names need to
have `bringup: true`.


----

Original PR description:

This cleans up the data_assets experiment testing.

* Use integration test projects, which are statically analyzed,
formatted, etc.
* This forces us to be explicit about the `package:data_assets`
dependency.
* This means the test projects will roll their dependencies with the
dependency auto roller. (The test project was still using
`native_assets_cli` which has been replaced with `hooks` and
`data_assets`.)
* Pin `package:data_assets`, because it's experimental, and we might do
breaking changes.

Also makes the test projects more useful:

* The `data_assets_app` now displays the assets in the UI. (Not only
prints to the console for the integration tests.)
* The `data_assets_package` now actually uses its asset.

And cleans up the test projects:

* Move the test files into the `data/` dir.
* Give the test data assets a proper extension.

I saw weird behavior on Windows:

* DevFS doesn't pick up the writes to the Dart files, not even with
flushing or sleeps: `Compiling dart to kernel with 0 updated files`
leading to a state where the Dart variable contents don't get updated
while the data assets do. This integration test is not really about Dart
hot restart, so I've skipped those checks on Windows.
* This reproduces locally in the integration test, but not when running
the app and editing the file.

The checks in dev/bots/analyze.dart also required some updates:

* The template files check didn't allow for the HTML license.

P.S. Sorry for all the generated Flutter scaffolding. It seems that that
is the way it's done in the integration test directory. (The package
with the data assets is clean, but the Flutter app ...)
creatorpiyush pushed a commit to creatorpiyush/packages that referenced this pull request Jun 10, 2026
…r#11463)

flutter/flutter@9cd60b5...a0924c7

2026-04-07 [email protected] Reland "[data_assets] Cleanup tests" (flutter/flutter#184714)
2026-04-07 [email protected] Use the WindowRegistry in the multiple_windows example app (flutter/flutter#184579)
2026-04-07 [email protected] Introduce command to build a swift package for SwiftPM add to app integration (flutter/flutter#184660)
2026-04-07 [email protected] Have `flutter create` create a pubspec.lock to ensure pinned versions are being used. (flutter/flutter#175352)
2026-04-07 [email protected] [widgets/raw_menu_anchor.dart] Always call onClose and onCloseRequested on descendants before parent. (flutter/flutter#182357)
2026-04-07 [email protected] `WindowsPlugin` should not crash when ffiPlugin enabled (flutter/flutter#184695)
2026-04-06 [email protected] Use full goto.google.com hostname for go/ links (flutter/flutter#184679)
2026-04-06 [email protected] Apply rect clipping to surface views (flutter/flutter#184471)
2026-04-06 [email protected] [A11y] Allow percentage strings like "50%" as `SemanticsValue` for `ProgressIndicator` (flutter/flutter#183670)
2026-04-06 [email protected] Fix invisible accessibility element before scroll view (flutter/flutter#184155)
2026-04-06 [email protected] Roll Skia from 163dfdf500c7 to e264d870a380 (2 revisions) (flutter/flutter#184674)
2026-04-06 [email protected] Keep last character obscured when toggling obscureText (flutter/flutter#183488)
2026-04-06 [email protected] Parse scheme file with XML parser for SwiftPM migrator (flutter/flutter#184525)

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],[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
pull Bot pushed a commit to Mu-L/flutter that referenced this pull request Sep 12, 2026
Fixes flutter#191628
Fixes flutter#187204

Bumps Dart across the repo to 3.13 and reformats the repo
The CI formatting check is also re-enabled with this change.

#### Deviating pubspec.yaml Packages
The following packages deviate from the standard repository Dart SDK
constraint (^3.11.0-0 before the bump). Some of these cannot cleanly
downgrade. Changing them is considered out of scope. Some of them may
also pertain to active work that requires later versions of Dart.

1. ios_universal_link
- Full File Path: dev/integration_tests/ios_universal_link/pubspec.yaml
  - Dart SDK Version: ^3.14.0-53.0.dev
  - Author: @okorohelijah 
  - Commit: 2a071e2 (2026-09-04)
- PR / Message: Add iOS Universal Link integration test (Simulator)
(flutter#191443)
2. flutter_analyzer_plugin
  - Full File Path: dev/flutter_analyzer_plugin/pubspec.yaml
  - Dart SDK Version: ^3.7.0
  - Author: @bkonyi 
  - Commit: ae9963e (2026-08-12)
- PR / Message: [analysis] Reland "Added initial implementation of the
flutter_analyzer_plugin (flutter#175679)" (flutter#191022)
3. widget_preview_scaffold
- Full File Path:
dev/integration_tests/widget_preview_scaffold/pubspec.yaml
  - Dart SDK Version: ^3.12.0
  - Author: @polina-c 
  - Commit: 3ca99c3 (2026-05-15)
- PR / Message: [ Widget Preview ] Improve zoom behavior and add zoom
slider (flutter#186422)
4. hybrid_platform_views
- Full File Path:
dev/integration_tests/hybrid_android_views/pubspec.yaml
  - Dart SDK Version: ^3.7.0-0
  - Author: @andrewkolos 
  - Commit: 112f170 (2026-03-09)
  - PR / Message: Reland hybrid views test (flutter#181336)
5. record_use_test_app
- Full File Path: dev/integration_tests/record_use_test_app/pubspec.yaml
  - Dart SDK Version: >=3.5.0-0 <4.0.0
  - Author: @dcharkes 
  - Commit: eb6acab (2026-04-09)
- PR / Message: [record_use] Add experimental flag and test project
(flutter#184719)
6. record_use_test_package
- Full File Path:
dev/integration_tests/record_use_test_package/pubspec.yaml
  - Dart SDK Version: >=3.5.0-0 <4.0.0
  - Author: @dcharkes 
  - Commit: eb6acab (2026-04-09)
- PR / Message: [record_use] Add experimental flag and test project
(flutter#184719)
7. data_asset_app
  - Full File Path: dev/integration_tests/data_asset_app/pubspec.yaml
  - Dart SDK Version: >=3.5.0-0 <4.0.0
  - Author: @dcharkes 
  - Commit: a0924c7 (2026-04-07)
  - PR / Message: Reland "[data_assets] Cleanup tests" (flutter#184714)
8. data_asset_package
- Full File Path: dev/integration_tests/data_asset_package/pubspec.yaml
  - Dart SDK Version: >=3.5.0-0 <4.0.0
  - Author: @dcharkes 
  - Commit: a0924c7 (2026-04-07)
  - PR / Message: Reland "[data_assets] Cleanup tests" (flutter#184714)

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant in-code documentation (doc comments with
`///`).
- [x] If this PR introduces a new feature or capability, I created and
linked a website documentation issue or PR in [flutter/website] (or
verified none is needed).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

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](https://developers.google.com/gemini-code-assist/docs/review-github-code).
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.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[flutter/website]: https://github.com/flutter/website
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: desktop Running on desktop CICD Run CI/CD tool Affects the "flutter" command-line tool. See also t: labels. will affect goldens Changes to golden files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants