Add more structure to errors (continuation of #34684) #42640
Conversation
fluttergithubbot
added
f: cupertino
albertusdev
force-pushed
the
structured-errors
branch
5 times, most recently
from
c8b8556 to
87efe7c
Compare
…Painter, TextSpan
albertusdev
force-pushed
the
structured-errors
branch
from
87efe7c to
bb01db5
Compare
albertusdev
force-pushed
the
structured-errors
branch
from
bb01db5 to
8dbadb8
Compare
| // LayoutDelegate that layout child with id 0 and 1 | ||
| // Used in the 'performLayout error control test' test case for triggering | ||
| // For triggering an error when laying out non existent child | ||
| // For triggering an error when a child has not been laid out |
There was a problem hiding this comment.
"For triggering an error when a child has not been laid out" ==> "and a child that has not been laid out"
packages/flutter/test/widgets/custom_multi_child_layout_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter/test/widgets/custom_multi_child_layout_test.dart
Outdated
Show resolved
Hide resolved
| 'All SemanticsHandle instances must be disposed by calling dispose() on ' | ||
| 'the SemanticsHandle.' | ||
| ), | ||
| ErrorHint( |
There was a problem hiding this comment.
Fyi @InMatrix
added an extra line break here. This seems like a reasonable hint.
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
The tests for this code look really good overall!
Really an impressive job of testing a wide range of Widget and Render errors that had zero test coverage.
Lgtm from a Flutter DevExp perspective.
These error messages will really help us do a better job explaining to users what went wrong with their layouts.
Adding @goderbauer for further review and fyi @InMatrix who will be excited to see this code land.
@goderbauer the good news is that about 2/3 of this code is tests so it isn't quite as painful to review as the line lengths suggest.
Signed-off-by: Albertus Angga Raharja <[email protected]>
Signed-off-by: Albertus Angga Raharja <[email protected]>
Signed-off-by: Albertus Angga Raharja <[email protected]>
Signed-off-by: Albertus Angga Raharja <[email protected]>
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
RSLGTM
I did not review every single line of this PR, but what I saw looked good.
Signed-off-by: Albertus Angga Raharja <[email protected]>
| '${activeCurve.runtimeType} mapped $t to $transformedValue, which ' | ||
| 'is near $roundedTransformedValue.' | ||
| ) | ||
| ]); |
There was a problem hiding this comment.
I thought FlutterError would automatically take its first line and use it as a summary and use the rest as a description, is that not the case?
There was a problem hiding this comment.
Yes I believe so too, this is actually redundant change
cc: @jacob314
There was a problem hiding this comment.
Yes go ahead and revert this change as part of your followup cleanup CL.
| }); | ||
|
|
||
| testWidgets('verifyTickersWereDisposed control test', (WidgetTester tester) async { | ||
| FlutterError error; |
There was a problem hiding this comment.
yes sorry for that :( should I make new PR to fix this?
There was a problem hiding this comment.
no worries. go ahead and create a new PR with just the whitespace fix. Good news is whitespace only CLs are really quick to review. :)
| 'existing handle will leak into another test and alter its behavior.' | ||
| ); | ||
| // TODO(jacobr): The hint for this one causes a change in line breaks but | ||
| // I think it is for the best. |
There was a problem hiding this comment.
not sure what this TODO means?
| '$runtimeType does not have a child widget but received a non-empty list of child SemanticsNode:\n' | ||
| '${children.join('\n')}' | ||
| ); | ||
| throw FlutterError.fromParts(<DiagnosticsNode>[ |
There was a problem hiding this comment.
I think we missed this one, this one is changing everything to be summary, is this intended?
@jacob314
There was a problem hiding this comment.
good catch. Only the first line should be the summary.
…#42640) * Add structured errors in Animations, TabView, ChangeNotifier * Add structured error on MaterialPageRoute, BoxBorder, DecorationImagePainter, TextSpan * Add structured errors in Debug * Fix test errors * Add structured errors in Scaffold and Stepper * Add structured errors in part of Rendering Layer * Fix failing test due to FloatingPoint precision * Fix failing tests due to precision error and not using final * Fix failing test due to floating precision error with RegEx instead * Add structured error in CustomLayout and increase test coverage * Add structured error & its test in ListBody * Add structured error in ProxyBox and increase test coverage * Add structured error message in Viewport * Fix styles and add more assertions on ErrorHint and DiagnosticProperty * Add structured error in scheduler/binding and scheduler/ticker Signed-off-by: Albertus Angga Raharja <[email protected]> * Add structured error in AssetBundle and TextInput Signed-off-by: Albertus Angga Raharja <[email protected]> * Add structured errors in several widgets #1 Signed-off-by: Albertus Angga Raharja <[email protected]> * Remove unused import Signed-off-by: Albertus Angga Raharja <[email protected]> * Add assertions on hint messages Signed-off-by: Albertus Angga Raharja <[email protected]> * Fix catch spacing Signed-off-by: Albertus Angga Raharja <[email protected]> * Add structured error in several widgets part 2 and increase code coverage Signed-off-by: Albertus Angga Raharja <[email protected]> * Add structured error in flutter_test/widget_tester * Fix floating precision accuracy by using RegExp Signed-off-by: Albertus Angga Raharja <[email protected]> * Remove todo to add tests in Scaffold showBottomSheet Signed-off-by: Albertus Angga Raharja <[email protected]> * Fix reviews by indenting lines and fixing the assertion orders Signed-off-by: Albertus Angga Raharja <[email protected]> * Fix failing tests due to renaming class Signed-off-by: Albertus Angga Raharja <[email protected]> * Try skipping the NetworkBundleTest Signed-off-by: Albertus Angga Raharja <[email protected]> * Remove leading space in material/debug error hint Signed-off-by: Albertus Angga Raharja <[email protected]>
This PR is a follow up of #42640 Some changes of that PR includes redundant changes using FlutterError.fromParts constructor even though it's not necessary. Some minor changes are: - Remove one unnecessary todo - Fix indent consistencies
7 participants
Description
This CL refactors more FlutterError messages to take advantage of structured error functionality and also increase the test coverage for all of the FlutterError that got refactored.
This is a follow up to #34684
Related Issues
Tests
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?