-
Notifications
You must be signed in to change notification settings - Fork 346
Inspector integration tests are resilient to Flutter framework changes #9321
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from my side 👍. Just some questions below.
I wonder how many users of the framework end up doing similar things.
// Implementation widgets from Flutter framework are not guaranteed to | ||
// be stable. | ||
skip: 'https://github.com/flutter/flutter/issues/172037', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what you guys were hoping to test with these goldens and if it can be achieved with widget tests instead?
Taking a look at a golden image here (https://github.com/flutter/devtools/blob/master/packages/devtools_app/test/test_infra/goldens/integration_inspector_select_center_details_tree.png) it looks pretty UI-light, which is generally better for golden testing, but still I usually avoid golden testing if possible due to unintended breakages like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the golden tests are mostly to make sure that we don't accidentally introduce some UI change that we wouldn't have thought to test in our widget tests. Since this specific view will be going away, I'm not too concerned about not having comprehensive test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the V2 view, I think the fact that we have golden tests for the panel (just without implementation widgets enabled) should catch the vast majority of UI regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you are working around the fact that Flutter might make breaking changes to these widgets? On the framework team we're currently thinking about what widgets should be in the Widgets library (and more stable), so maybe this helps inform us there.
I'm seeing that you made replacements for these widgets:
- MaterialApp
- Container
- Center
- Text
- TextButton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly! If there are widgets in the Widgets library that are guaranteed to be stable that would be good to know, since we could make sure our app only contains those widgets.
Work towards flutter/flutter#172037
Our current inspector integration test is quite brittle, since the test cases for the inspector tree assume the implementation widgets from the Flutter frameworks will be in a consistent order. This PR removes that assumption to make sure the inspector tests won't fail due to framework changes.
This PR:
custom_widgets
) to our test fixtures which contains a few simple "custom" widgetsinspector_app
) to our test fixtures which uses the widgets from thecustom_widgets
library instead of from the Frameworkinspector_v2/inspector_integration_test
to use the newinspector_app
This way we can rely on the implementing widgets from
custom_widgets
remaining consistent. However, since those are implemented using widgets from the framework, I have skipped any golden comparisons when implementation widgets are visible in the tree.Note: We are planning on removing the legacy inspector (integration test is
packages/devtools_app/test/screens/inspector/inspector_integration_test.dart
) soon now that ~99% of users are using the new inspector. Therefore, for the legacy inspector integration test, I simply skipped any golden comparisons for the details tree.