[flutter_tools] fix: don't rebuild unit_test_assets when nothing changed#187751
[flutter_tools] fix: don't rebuild unit_test_assets when nothing changed#187751hkarmoush wants to merge 2 commits into
Conversation
DevFSFileContent.isModifiedAfter() returned true whenever _fileStat was null (i.e. markClean() had never been called). Since _needsRebuild() in test.dart inspects freshly-constructed AssetBundleEntry objects that never have markClean() called, every entry reported "modified" and the asset directory was deleted and rewritten on every flutter test run. When two flutter test invocations share a project directory concurrently one invocation's delete lands while the other's flutter_tester processes are reading an asset, causing "Asset not found" failures. Fix: isModifiedAfter now compares the file's current mtime directly against the provided time, with no dependency on the _fileStat baseline. This matches DevFSByteContent and DevFSStringCompressingBytesContent. Fixes flutter#187725 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request simplifies the DevFSFileContent.isModifiedAfter check to prevent unnecessary asset bundle rebuilds when no changes have occurred, and adds corresponding regression tests. Feedback suggests returning true instead of false when currentStat is null to ensure file deletions are correctly detected as modifications, adding a test assertion for this deletion case, and refactoring the test to use the local fs instance instead of globals.fs.
When currentStat is null (file deleted from disk), isModifiedAfter was returning false, meaning asset deletions would not trigger a rebuild and stale assets could persist in the unit_test_assets directory. Return true for the null case so deletions are treated as modifications, consistent with the conservative "rebuild when in doubt" policy of a build system. Also add a regression test asserting the deletion case, and switch from globals.fs to the local fs instance in test_test.dart per style guidelines. Addresses review feedback on PR flutter#187751. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Description
Fixes #187725.
flutter testwas rebuildingbuild/unit_test_assetson every invocation, even when nothing changed. When twoflutter testinvocations run concurrently in the same project directory, one invocation's delete lands while the other'sflutter_testerprocesses are reading an asset, causing failures like:Root cause
DevFSFileContent.isModifiedAfter()returnedtruewhenever_fileStat == null. Since_fileStatis only populated bymarkClean(), and_needsRebuild()intest.dartinspects freshly-constructedAssetBundleEntryobjects that never havemarkClean()called, every asset entry reported "modified" on every run.This is a regression from #187488 which made the stat accessors pure (
_stat()→_statFile()). Before that, the mutating_stat()populated_fileStatas a side effect during the asset build, so the baseline happened to be set when_needsRebuild()ran.Fix
isModifiedAfter(time)now drops all reference to_fileStatand compares the file's current mtime directly againsttime. This matches the pattern inDevFSByteContentandDevFSStringCompressingBytesContent, which both compare a snapshot time againsttimewith no baseline dependency.Tests
devfs_test.dart: unit test callingisModifiedAfteron a freshDevFSFileContentwith no priormarkClean()— the exact path the bug hid in.test_test.dart: regression test running the command twice with no source changes, injecting a sentinel into the built output between runs, and asserting the sentinel survives (proving no rebuild occurred).Pre-submission checklist
///?).