Expand coverage of android_hardware_smoke_test. Add image, text, blend mode, and blur tests.#187600
Expand coverage of android_hardware_smoke_test. Add image, text, blend mode, and blur tests.#187600andywolff wants to merge 6 commits into
Conversation
- Add 'imageTest' rendering test case. - Add a small 32x32 colored checkerboard test png. - Add image loading behavior which executes only for the relevant test case, and handles load failure. - Support dependency injection for ImageLoader to enable mocking in widget tests. - Add 'captureScreenshot' channel parameter to skip fetching image bytes, which would cause a FakeAsync deadlock in widget tests. - Dispose of native Codec and Image handles in main.dart and goldens.dart to prevent resource leaks which can cause widget tests to hang.
…t so far and any references which informed them
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request expands the android_hardware_smoke_test integration test suite by introducing new test cases for text rendering, image texture sampling, advanced blending, and backdrop filter blurring. It refactors the test application to support these scenarios with new widgets and lazy image loading, and adds corresponding widget and driver tests. Feedback focuses on addressing potential runtime exceptions and memory leaks by checking if the widget is mounted before calling setState during image loading, avoiding force-unwrapping on potentially null images, and documenting or privatizing several undocumented public members to comply with the repository's style guide.
| if (testName == 'imageTest' && _loadedImage == null) { | ||
| try { | ||
| final ui.Image img = await _loadImage(); | ||
| setState(() { | ||
| _loadedImage = img; | ||
| }); | ||
| } catch (e, stackTrace) { | ||
| return <String, Object?>{'message': 'Failed to load image asset: $e\n$stackTrace'}; | ||
| } | ||
| } |
There was a problem hiding this comment.
If the widget is disposed while _loadImage() is in progress, calling setState will throw a runtime exception. Additionally, the loaded ui.Image will not be assigned to _loadedImage and will be leaked. Checking mounted before calling setState and disposing the image if the widget is no longer mounted prevents these issues.
if (testName == 'imageTest' && _loadedImage == null) {
try {
final ui.Image img = await _loadImage();
if (!mounted) {
img.dispose();
return <String, Object?>{'message': 'Widget was disposed before image loaded.'};
}
setState(() {
_loadedImage = img;
});
} catch (e, stackTrace) {
return <String, Object?>{'message': 'Failed to load image asset: $e\n$stackTrace'};
}
}| Future<ui.Image> defaultImageLoader() async { | ||
| final ByteData data = await rootBundle.load('assets/test_image.png'); | ||
| final ui.Codec codec = await ui.instantiateImageCodec(data.buffer.asUint8List()); | ||
| final ui.FrameInfo fi = await codec.getNextFrame(); | ||
| codec.dispose(); | ||
| return fi.image; | ||
| } | ||
|
|
||
| void main() async { | ||
| runApp(const MyApp()); | ||
| } | ||
|
|
||
| class MyApp extends StatelessWidget { | ||
| const MyApp({super.key}); | ||
| const MyApp({super.key, this.imageLoader = defaultImageLoader}); | ||
|
|
||
| final ImageLoader imageLoader; |
There was a problem hiding this comment.
The defaultImageLoader function is a public member but lacks documentation, which violates the style guide. Since it is only used as the default parameter for MyApp, we can make it private to keep the public API clean and avoid the need for public documentation.
| Future<ui.Image> defaultImageLoader() async { | |
| final ByteData data = await rootBundle.load('assets/test_image.png'); | |
| final ui.Codec codec = await ui.instantiateImageCodec(data.buffer.asUint8List()); | |
| final ui.FrameInfo fi = await codec.getNextFrame(); | |
| codec.dispose(); | |
| return fi.image; | |
| } | |
| void main() async { | |
| runApp(const MyApp()); | |
| } | |
| class MyApp extends StatelessWidget { | |
| const MyApp({super.key}); | |
| const MyApp({super.key, this.imageLoader = defaultImageLoader}); | |
| final ImageLoader imageLoader; | |
| Future<ui.Image> _defaultImageLoader() async { | |
| final ByteData data = await rootBundle.load('assets/test_image.png'); | |
| final ui.Codec codec = await ui.instantiateImageCodec(data.buffer.asUint8List()); | |
| final ui.FrameInfo fi = await codec.getNextFrame(); | |
| codec.dispose(); | |
| return fi.image; | |
| } | |
| void main() async { | |
| runApp(const MyApp()); | |
| } | |
| class MyApp extends StatelessWidget { | |
| const MyApp({super.key, this.imageLoader = _defaultImageLoader}); | |
| /// The image loader used to fetch the image asset. | |
| final ImageLoader imageLoader; |
References
- All public members should have documentation. (link)
| const MyWidget({super.key, required this.imageLoader}); | ||
|
|
||
| final ImageLoader imageLoader; |
There was a problem hiding this comment.
The imageLoader field in MyWidget is a public member but lacks documentation. Please add a doc comment to describe its purpose.
| const MyWidget({super.key, required this.imageLoader}); | |
| final ImageLoader imageLoader; | |
| const MyWidget({super.key, required this.imageLoader}); | |
| /// The image loader used to fetch the image asset. | |
| final ImageLoader imageLoader; |
References
- All public members should have documentation. (link)
| class BackgroundPainter extends CustomPainter { | ||
| const BackgroundPainter(); |
There was a problem hiding this comment.
The BackgroundPainter class is a public member but lacks documentation. Since it is only used internally within BackdropFilterBlur, we should make it private to keep the public API clean and avoid the need for public documentation. Remember to also update the usage on line 17 to _BackgroundPainter.
| class BackgroundPainter extends CustomPainter { | |
| const BackgroundPainter(); | |
| class _BackgroundPainter extends CustomPainter { | |
| const _BackgroundPainter(); |
References
- All public members should have documentation. (link)
| class VectorDrawingsCanvas extends StatelessWidget { | ||
| const VectorDrawingsCanvas({super.key, required this.message, this.loadedImage}); | ||
|
|
||
| final String message; | ||
| final ui.Image? loadedImage; |
There was a problem hiding this comment.
The message and loadedImage fields in VectorDrawingsCanvas are public members but lack documentation. Please add doc comments to describe their purpose.
class VectorDrawingsCanvas extends StatelessWidget {
const VectorDrawingsCanvas({super.key, required this.message, this.loadedImage});
/// The active test scenario command.
final String message;
/// The pre-loaded image to render, if any.
final ui.Image? loadedImage;References
- All public members should have documentation. (link)
| void renderImageTest(Canvas canvas, Size size) { | ||
| assert(loadedImage != null); | ||
| canvas.drawImage(loadedImage!, Offset.zero, Paint()); | ||
| } |
There was a problem hiding this comment.
To enforce defensive programming and avoid a potential runtime crash if loadedImage is null, we should check if loadedImage is non-null before calling drawImage instead of using a force-unwrap (!).
void renderImageTest(Canvas canvas, Size size) {
final ui.Image? image = loadedImage;
if (image != null) {
canvas.drawImage(image, Offset.zero, Paint());
}
}
Expand test coverage to address more of #182123
Platform view tests will be next after this.
Pre-launch Checklist
///).