Improve exceptions thrown by asset bundle#114139
Improve exceptions thrown by asset bundle#114139AlexV525 wants to merge 14 commits intoflutter:masterfrom
Conversation
flutter-dashboard
bot
added
the
framework
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
We should add a test for this, just for completeness. Maybe while we're at it we could add more detail? Not sure if there's any more detail to add though. |
|
Wow I can't even find proper tests with |
| @@ -82,7 +82,7 @@ abstract class AssetBundle { | |||
| Future<String> loadString(String key, { bool cache = true }) async { | |||
| final ByteData data = await load(key); | |||
| if (data == null) { | |||
There was a problem hiding this comment.
This should be removed. But the analyzer doesn't report with an unnecessary null check.
AlexV525
changed the title
. when throwing exceptions in asset bundle
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| }, skip: isBrowser); // https://github.com/flutter/flutter/issues/39998 | ||
|
|
||
| test('Throws expected exceptions when loading not exists asset', () async { | ||
| TestWidgetsFlutterBinding.ensureInitialized(); |
There was a problem hiding this comment.
One strange thing I found during tests is if we call the ensureInitialized in this test file, the "NetworkAssetBundle control test" will fail because the request returns 400 instead of 404.
There was a problem hiding this comment.
the widget tester overrides HTTP calls to always return 400.
There was a problem hiding this comment.
the widget tester overrides HTTP calls to always return 400.
TIL.
There was a problem hiding this comment.
Eventually I update the test to use 400 instead.
| /// isolate to avoid jank on the main thread. | ||
| Future<String> loadString(String key, { bool cache = true }) async { | ||
| final ByteData data = await load(key); | ||
| if (data == null) { |
There was a problem hiding this comment.
Don't we want to keep this here?
There was a problem hiding this comment.
You see, the data here is not nullable, so it should be a unnecessary check IMO.
There was a problem hiding this comment.
And I'm also wondering why the analyzer doesn't report about it.
There was a problem hiding this comment.
Ahhh interesting, ok. Maybe because of unsound null safety?
There was a problem hiding this comment.
I'll take another look to see if there are any issues with the linter rule.
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
3 participants
Consist messages when throwing from asset bundles.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.