Catch any FileSystemException thrown when trying to read the template manifest during flutter create
#145620
Conversation
github-actions
bot
added
the
tool
|
@andrewkolos what's the status of this? is this ready for review? |
andrewkolos
force-pushed
the
catch-filesystemexception-when-reading-template-manifest
branch
from
a2da712 to
68873b0
Compare
| testUsingContext('flutter create should tool exit if the template manifest cannot be read', () async { | ||
| final CreateCommand command = CreateCommand(); | ||
| final CommandRunner<void> runner = createTestCommandRunner(command); | ||
| projectDir.createSync(recursive: true); | ||
| await expectLater( | ||
| runner.run( | ||
| <String>['create', '--no-pub', '--template=plugin', projectDir.path]), | ||
| throwsToolExit(message: 'Unable to read the template manifest at path'), | ||
| ); | ||
| }, overrides: <Type, Generator>{ | ||
| FileSystem: () => MemoryFileSystem.test( | ||
| opHandle: (String context, FileSystemOp operation) { | ||
| if (context.contains('template_manifest.json')) { | ||
| throw PathNotFoundException( | ||
| context, const OSError(), 'Cannot open file'); | ||
| } | ||
| }, | ||
| ), | ||
| ProcessManager: () => fakeProcessManager, | ||
| }); |
There was a problem hiding this comment.
Is this test working as you expected?
final String manifestPath = globals.fs.path.join(
flutterToolsAbsolutePath,
'templates',
'template_manifest.json',
);The line above is from create_base.dart, looks like you would always get a filesystem exception because that file doesn't exist in the memory file system right?
There was a problem hiding this comment.
Yeah just confirmed that if you comment out the throw from the opHandle, you still end up having it get caught because it is failing to find the template file
There was a problem hiding this comment.
You are correct, but I am not sure there is a clearly better alternative. We could remove the custom opHandle provided to the MemoryFileSystem instance and replace it with a code comment explaining why the FileSystemException will get thrown. Do you think this would be a better idea?
There was a problem hiding this comment.
I'm not entirely sure what the best alternative would be either, but also when you try to check the runtimeType for globals.fs, it's still the ErrorHandlingFileSystem instead of MemoryFileSystem... are we expecting globals.fs to point to your memory fs in the overrides?
There was a problem hiding this comment.
I don't understand what you mean by "checking the runtime type for globals.fs"? Could you provide a code suggestion of what you are talking about?
globals.fs will indeed resolve to the MemoryFileSystem instance produced by the factory function we provided in the overrides map.
There was a problem hiding this comment.
Sorry i confused myself, the globals.fs will always point to the ErrorHandlingFileSystem, its the delegate that we end up overriding in the test overrides.
But to answer your question, i'm not sure if adding a comment is the best approach either because the test is not really testing what you just added. Can we instead write the template file into the MemoryFileSystem so that it can find it and then your throw will actually happen?
There was a problem hiding this comment.
This is also under test/commands.shard/permeable so you could also extend LocalFileSystem to throw your error and also guarantee you have template file in the filesystem.
This was done for some other tests before too: https://github.com/flutter/flutter/blob/main/packages/flutter_tools/test/general.shard/resident_runner_helpers.dart#L187-L197
There was a problem hiding this comment.
Sorry i confused myself, the
globals.fswill always point to theErrorHandlingFileSystem, its the delegate that we end up overriding in the test overrides.
Oh yeahhh I forgot about that, which is why I couldn't figure out what you were saying at first 😆.
But to answer your question, i'm not sure if adding a comment is the best approach either because the test is not really testing what you just added. Can we instead write the template file into the MemoryFileSystem so that it can find it and then your
throwwill actually happen?
Sure, I've done this. Let me know whether I understood correctly.
andrewkolos
requested review from
christopherfujino and
eliasyishak
and removed request for
eliasyishak
packages/flutter_tools/test/commands.shard/permeable/create_test.dart
Outdated
Show resolved
Hide resolved
christopherfujino
left a comment
christopherfujino
left a comment
There was a problem hiding this comment.
2 nit questions, but LGTM
andrewkolos
added
the
autosubmit
…e template manifest during `flutter create` (flutter/flutter#145620)
…e template manifest during `flutter create` (flutter/flutter#145620)
flutter/flutter@2e80670...f9933b6 2024-04-27 [email protected] Roll Flutter Engine from 5205e3683a0a to 20fb62ba1455 (1 revision) (flutter/flutter#147449) 2024-04-27 [email protected] Roll Flutter Engine from e14649ea0c80 to 5205e3683a0a (1 revision) (flutter/flutter#147448) 2024-04-27 [email protected] Roll Flutter Engine from 87f489c1bed4 to e14649ea0c80 (1 revision) (flutter/flutter#147446) 2024-04-27 [email protected] Roll Flutter Engine from cecf5aa8a778 to 87f489c1bed4 (1 revision) (flutter/flutter#147445) 2024-04-27 [email protected] Roll Flutter Engine from 8af10eba3ef3 to cecf5aa8a778 (1 revision) (flutter/flutter#147444) 2024-04-27 [email protected] [macOS] Eliminate flutter_gallery_macos__start_up benchmark (flutter/flutter#147442) 2024-04-27 [email protected] Roll Flutter Engine from bc055398f42a to 8af10eba3ef3 (1 revision) (flutter/flutter#147441) 2024-04-27 [email protected] Roll Flutter Engine from c410180e5bba to bc055398f42a (7 revisions) (flutter/flutter#147440) 2024-04-26 [email protected] Add tests for character_activator.0.dart API example. (flutter/flutter#147384) 2024-04-26 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.2 to 3.25.3 (flutter/flutter#147437) 2024-04-26 [email protected] Add integration test for asset transformation feature (flutter/flutter#145715) 2024-04-26 [email protected] Added missing tests for Table api example `table.0.dart`. (flutter/flutter#147318) 2024-04-26 [email protected] Catch any `FileSystemException` thrown when trying to read the template manifest during `flutter create` (flutter/flutter#145620) 2024-04-26 [email protected] fixes `CupertinoFullscreenDialogTransition` leaks (flutter/flutter#147168) 2024-04-26 [email protected] Fix helperMaxLines and errorMaxLines documentation (flutter/flutter#147409) 2024-04-26 [email protected] Refactor route focus node creation (flutter/flutter#147390) 2024-04-26 [email protected] Roll Packages from fde908d to dd01140 (5 revisions) (flutter/flutter#147420) 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
deleted the
catch-filesystemexception-when-reading-template-manifest
branch
|
Was this expected to demonstrate some improvements on the startup test? |
I doubt it, I would expect if we're hitting this code path during the test, the test runner would just crash... Is it possible skia perf ran the commits out of order, and the previous commit improved this benchmark (but ran after)? |
|
That can happen. But in this case the 2 commits next to it in the graph are the 2 commits that flank it in the repo... Curious... I triaged it, because hey, we'll take the win. But it's always reassuring if we know why. |
flutter/flutter@2e80670...f9933b6 2024-04-27 [email protected] Roll Flutter Engine from 5205e3683a0a to 20fb62ba1455 (1 revision) (flutter/flutter#147449) 2024-04-27 [email protected] Roll Flutter Engine from e14649ea0c80 to 5205e3683a0a (1 revision) (flutter/flutter#147448) 2024-04-27 [email protected] Roll Flutter Engine from 87f489c1bed4 to e14649ea0c80 (1 revision) (flutter/flutter#147446) 2024-04-27 [email protected] Roll Flutter Engine from cecf5aa8a778 to 87f489c1bed4 (1 revision) (flutter/flutter#147445) 2024-04-27 [email protected] Roll Flutter Engine from 8af10eba3ef3 to cecf5aa8a778 (1 revision) (flutter/flutter#147444) 2024-04-27 [email protected] [macOS] Eliminate flutter_gallery_macos__start_up benchmark (flutter/flutter#147442) 2024-04-27 [email protected] Roll Flutter Engine from bc055398f42a to 8af10eba3ef3 (1 revision) (flutter/flutter#147441) 2024-04-27 [email protected] Roll Flutter Engine from c410180e5bba to bc055398f42a (7 revisions) (flutter/flutter#147440) 2024-04-26 [email protected] Add tests for character_activator.0.dart API example. (flutter/flutter#147384) 2024-04-26 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.2 to 3.25.3 (flutter/flutter#147437) 2024-04-26 [email protected] Add integration test for asset transformation feature (flutter/flutter#145715) 2024-04-26 [email protected] Added missing tests for Table api example `table.0.dart`. (flutter/flutter#147318) 2024-04-26 [email protected] Catch any `FileSystemException` thrown when trying to read the template manifest during `flutter create` (flutter/flutter#145620) 2024-04-26 [email protected] fixes `CupertinoFullscreenDialogTransition` leaks (flutter/flutter#147168) 2024-04-26 [email protected] Fix helperMaxLines and errorMaxLines documentation (flutter/flutter#147409) 2024-04-26 [email protected] Refactor route focus node creation (flutter/flutter#147390) 2024-04-26 [email protected] Roll Packages from fde908d to dd01140 (5 revisions) (flutter/flutter#147420) 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
…e template manifest during `flutter create` (flutter/flutter#145620)
4 participants
Fixes #145423
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.