Skip to content

Deflake wildcard asset test #42597

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jonahwilliams merged 2 commits into from
Oct 14, 2019
Merged

Deflake wildcard asset test #42597

jonahwilliams merged 2 commits into from
Oct 14, 2019

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Oct 12, 2019

Description

For wildcard assets, filestat the individual contents instead of the directory. Deflake the test by manually setting the lastModifiedSync to the future.

Fixes #42230
Fixes #34446

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 12, 2019
@codecov
Copy link

codecov bot commented Oct 12, 2019 •
edited
Loading

Codecov Report

Merging #42597 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #42597      +/-   ##
==========================================
- Coverage   59.94%   59.94%   -0.01%     
==========================================
  Files         194      194              
  Lines       18871    18873       +2     
==========================================
  Hits        11313    11313              
- Misses       7558     7560       +2
Flag Coverage Δ
#flutter_tool 59.94% <100%> (-0.01%) ⬇️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/asset.dart 86.13% <100%> (+2.23%) ⬆️
packages/flutter_tools/lib/src/ios/simulators.dart 11.23% <0%> (-39.5%) ⬇️
...ckages/flutter_tools/lib/src/flutter_manifest.dart 77.94% <0%> (-6.67%) ⬇️
packages/flutter_tools/lib/src/version.dart 93.23% <0%> (-1.94%) ⬇️
...ckages/flutter_tools/lib/src/base/file_system.dart 66.66% <0%> (-1.86%) ⬇️
...ges/flutter_tools/lib/src/application_package.dart 65.94% <0%> (-1.09%) ⬇️
packages/flutter_tools/lib/src/cache.dart 48.47% <0%> (-0.71%) ⬇️
packages/flutter_tools/lib/src/vmservice.dart 41.06% <0%> (-0.17%) ⬇️
...ackages/flutter_tools/lib/src/resident_runner.dart 57.37% <0%> (+0.61%) ⬆️
packages/flutter_tools/lib/src/device.dart 66.86% <0%> (+1.2%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fea283...a9b99dd. Read the comment docs.

@jonahwilliams jonahwilliams requested a review from dnfield October 14, 2019 13:54
dnfield
dnfield approved these changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

This should deflake the test but this seems like the implementation could end up going wrong

How much more expensive would it be to hash the files?

@jonahwilliams
Copy link
Contributor Author

jonahwilliams commented Oct 14, 2019

For the flutter gallery it takes about half a second. For an application with larger assets or on a slower compute it takes significantly longer. Keep in mind we would need to hash every single asset file on hot reloads and hot restarts.

The consequences of getting this wrong during development are pretty mild, just hot restart again. For building applications we use the file hashes instead of timestamps

The trade-off isn't really worth it.

@jonahwilliams jonahwilliams merged commit 04e04ff into flutter:master Oct 14, 2019
@jonahwilliams jonahwilliams deleted the fix_wildcard_test branch October 14, 2019 15:49
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@Inconnu08
Deflake wildcard asset test (flutter#42597)
7703f7b
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AssetBundle.needsBuild(..) ignores deleted wildcard directories. flutter_tools\test\asset_bundle_test.dart is flaky on windows

4 participants

@jonahwilliams @dnfield @googlebot @fluttergithubbot