Skip to content

Conversation

@andrewkolos
Copy link
Contributor

Part of work on #141214. See this discussion for the inspiration for this PR.

Issue

Many tests in packages/flutter_tools/test/general.shard/asset_bundle_test.dart aren't hermetic. When setting up fake FileSystem and Platform objects, the host OS is referenced:

setUp(() async {
testFileSystem = MemoryFileSystem(
style: globals.platform.isWindows
? FileSystemStyle.windows
: FileSystemStyle.posix,
);

platform = FakePlatform(operatingSystem: globals.platform.operatingSystem);

To improve hermeticity here, we could instead run each once per valid combination of file system style and platform. However, it is unclear if these tests even depend on the file system style (integration tests should catch most cases where this might matter). As a result, I think it's sufficient to improve hermeticity by always assuming a Linux environment, which is generally our default (as MemoryFileSystem does, and most of our fakes of Platform do by default).

In general, if a test needs to run other kinds of environments, it should make this clear, ideally through the test name.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 17, 2024
@andrewkolos andrewkolos force-pushed the merge-asset-test-files branch from 3304072 to 0d2a5f1 Compare January 17, 2024 18:10
@andrewkolos andrewkolos changed the title Make test file systems/platforms used in asset_bundle_test.dart not dependent on the host platform Make test file systems/platforms used in asset_bundle_test.dart less dependent on the host platform Jan 17, 2024
@andrewkolos andrewkolos marked this pull request as ready for review January 17, 2024 18:45
@andrewkolos
Copy link
Contributor Author

@christopherfujino Let me know if this is sufficient in addressing your comment on #141214.

If we want to make these tests truly hermetic, ManifestAssetBundle would first need to be refactored to not be dependent on the global Cache.flutterRoot.

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 17, 2024
@auto-submit auto-submit bot merged commit 5757500 into flutter:master Jan 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 18, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jan 18, 2024
flutter/flutter@def6af0...f77f824

2024-01-18 [email protected] Roll Flutter Engine from 49fa2cb9024f to b75d6d80d813 (1 revision) (flutter/flutter#141771)
2024-01-18 [email protected] Roll Flutter Engine from 49c6ca211aa4 to 49fa2cb9024f (1 revision) (flutter/flutter#141762)
2024-01-18 [email protected] Roll Flutter Engine from 873449c27d5a to 49c6ca211aa4 (1 revision) (flutter/flutter#141760)
2024-01-18 [email protected] Roll Flutter Engine from bfdc0c5b2826 to 873449c27d5a (1 revision) (flutter/flutter#141759)
2024-01-18 [email protected] Catch UnsupportedError thrown when user provides an asset directory path containing invalid characters (flutter/flutter#141214)
2024-01-18 [email protected] Roll Flutter Engine from 48f89ac064ac to bfdc0c5b2826 (1 revision) (flutter/flutter#141752)
2024-01-18 [email protected] Roll Flutter Engine from 924c17245a78 to 48f89ac064ac (2 revisions) (flutter/flutter#141751)
2024-01-18 [email protected] Roll Flutter Engine from 98c16b430e6b to 924c17245a78 (1 revision) (flutter/flutter#141749)
2024-01-18 [email protected] Roll Flutter Engine from 73a2de5da53f to 98c16b430e6b (16 revisions) (flutter/flutter#141744)
2024-01-18 [email protected] Move mac pixel 7 pro test to presubmit: false (flutter/flutter#141747)
2024-01-18 [email protected] [web] prepare layers_test.dart for flutter/engine#49786 (flutter/flutter#141731)
2024-01-17 [email protected] Remove non-needed bot and increase time out for leak_tracking. (flutter/flutter#141712)
2024-01-17 [email protected] Add `headerHeight` for `SearchAnchor` (flutter/flutter#141223)
2024-01-17 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.23.0 to 3.23.1 (flutter/flutter#141715)
2024-01-17 [email protected] Make test file systems/platforms used in asset_bundle_test.dart less dependent on the host platform (flutter/flutter#141657)
2024-01-17 [email protected] Native assets: roll deps (flutter/flutter#141684)
2024-01-17 [email protected] Run build tests on both x64 and arm64. (flutter/flutter#141206)
2024-01-17 [email protected] Update tests to Xcode 15 (flutter/flutter#141706)
2024-01-17 [email protected] [web] prepare for flutter/engine#49786 (flutter/flutter#141700)
2024-01-17 [email protected] Marks Windows framework_tests_misc_leak_tracking to be unflaky (flutter/flutter#141676)
2024-01-17 [email protected] Label "flutter_localizations" PRs with "framework" (flutter/flutter#141654)
2024-01-17 [email protected] Fix Tooltip show delay when mouse moves to one Tooltip from another (flutter/flutter#141656)
2024-01-17 [email protected] Roll Packages from 7dd0fcb to 1a2b780 (6 revisions) (flutter/flutter#141683)
2024-01-17 [email protected] Fix the --empty flag to not try working with non-app templates (flutter/flutter#141632)
2024-01-17 [email protected] Revert "Roll Flutter Engine from 73a2de5da53f to c7e328518bc0 (5 revisions)" (flutter/flutter#141691)

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],[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
@andrewkolos andrewkolos deleted the merge-asset-test-files branch April 29, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants