[SuperEditor] Migrate golden tests (Resolves #2654)#2702
[SuperEditor] Migrate golden tests (Resolves #2654)#2702angelosilvestre wants to merge 1 commit into
Conversation
matthew-carroll
left a comment
matthew-carroll
left a comment
There was a problem hiding this comment.
I read through the new golden file and left some questions/thoughts about the current flutter_test_goldens API.
Also, any thoughts on your end based on your direct usage?
| testGoldenSceneOnMac('shows caret at right side of an image', (tester) async { | ||
| await Gallery( | ||
| tester, | ||
| sceneName: 'goldens/super-editor-image-caret-downstream-mac', |
There was a problem hiding this comment.
I think we wanna do something different here. The term sceneName doesn't imply a file path, but it is. There are three things related to this: the directory path, the file name, and a human readable description of the scene. This is a note for us to figure out those properties.
There was a problem hiding this comment.
Maybe we should default to place the file in a goldens directory. Since this the default in the golden_toolkit people are already used to it.
| ) | ||
| .itemFromPumper( | ||
| id: "1", | ||
| description: 'SuperEditor', |
There was a problem hiding this comment.
These descriptions are meant to be a short statement about what's captured in the golden. I'm guessing that "SuperEditor" isn't what this one should be.
| await tester.pump(); | ||
| }, | ||
| ) | ||
| .renderOrCompareGolden(); |
There was a problem hiding this comment.
Perhaps we should have a single shot API, too. I don't know if that's worth it or not, but obviously in this test we're not utilizing the gallery part of a Gallery.
There was a problem hiding this comment.
Yeah, I think it makes sense for single image tests.
| await tester.pump(); | ||
|
|
||
| await screenMatchesGolden(tester, 'super-editor-image-caret-downstream-ios'); | ||
| testGoldenSceneOnIOS('shows caret at right side of an image', (tester) async { |
There was a problem hiding this comment.
I'm wondering if we should try to rework the platform structure so that the platform is configured per golden image?
As I look at this I'm wondering, wouldn't it feel more appropriate to see the various platform versions of the caret all in one scene? It looks like most/all of the tests in this file could be a single scene image...
There was a problem hiding this comment.
It would be nice. We'll need to see how practical would that be. I guess this would only work when using itemFromPumper, right?
| testGoldensOniOS('from portrait to landscape', (tester) async { | ||
| testGoldenSceneOnIOS('from portrait to landscape', (tester) async { | ||
| tester.view | ||
| ..devicePixelRatio = 1.0 |
There was a problem hiding this comment.
Do you remember why we're making these changes to the test view here? The golden test runners set the pixel ratio to 1 for all goldens to reduce anti-aliasing.
| }) | ||
| .takePhoto(find.byType(SuperEditor), "landscape") | ||
| .renderOrCompareGolden( | ||
| goldenName: "super-editor-caret-rotation-portrait-landscape-ios", |
There was a problem hiding this comment.
I changed Gallery to include these configurations in the constructor. Should we do the same for FilmStrip?
There was a problem hiding this comment.
Yeah, I think it makes more sense to have this in the constructor.
| tester.view.physicalSize = screenSizeLandscape; | ||
| await tester.pumpAndSettle(); | ||
| }) | ||
| .takePhoto(find.byType(SuperEditor), "landscape") |
There was a problem hiding this comment.
Should we make the Finder optional and default to looking for a GoldenImageBounds to reduce the verbosity?
There was a problem hiding this comment.
I think it would be helpful.
The only downside for now that we have to add this to the tests: return scaffold(
_widgetTester,
GoldenImageBounds(
child: decorator != null //
? decorator.call(_widgetTester, superEditor)
: superEditor,
),
);But I don't have a better alternative at the moment. |
2 participants
No description provided.