Implement screenshot test for flutter web. #45530

chingjun · 2019-11-25T08:44:29Z

Description

Added a class WebGoldenComparator (corresponds to GoldenFileComparator for non-web), with default implementation being DefaultWebGoldenComparator.
DefaultWebGoldenComparator will issue a web request to the test server for taking and comparing the screenshot.
Added new files _matchers_io.dart and _matcher_web.dart, that contain the implementation of MatchesGoldenFile. On web, this matcher will first render the requested Element on screen, and defer the screenshot taking to the test server.
In flutter_test_platform.dart, add a flutter_golden handler for the test server, which does two things: screenshot taking and image comparison.
Screenshot is taken using WIP, and is clipped to only contain the Element requested.
For image comparison, it will start a flutter_tester process that loads flutter_test_config.dart file and uses the default goldenFileComparator to compare the golden images.
An additional FLUTTER_TEST_BROWSER environment variable is set when starting the flutter_tester for image comparison. SkiaClient is also updated to include this as one of the parameters.
Currently only one file is enabled for Flutter Web screenshot test, just to test the overall workflow. A followup PR will be sent to enable all screenshot tests for Flutter Web.

Related Issues

jonahwilliams · 2019-11-25T18:10:54Z

packages/flutter_test/lib/src/_goldens_io.dart

+
+  @override
+  Future<bool> compare(Element element, Size size, Uri golden) {
+    throw UnsupportedError('DefaultWebGoldenComparator is noonly supported on the web.');


Suggested change

throw UnsupportedError('DefaultWebGoldenComparator is noonly supported on the web.');

throw UnsupportedError('DefaultWebGoldenComparator is not supported on the web.');

I think this should read "only supported on the web".

Updated. It's actually only supported instead of `not supported though.

jonahwilliams · 2019-11-25T18:11:01Z

packages/flutter_test/lib/src/_goldens_io.dart

+
+  @override
+  Future<void> update(Uri golden, Element element, Size size) {
+    throw UnsupportedError('DefaultWebGoldenComparator is noonly supported on the web.');


Suggested change

throw UnsupportedError('DefaultWebGoldenComparator is noonly supported on the web.');

throw UnsupportedError('DefaultWebGoldenComparator is not supported on the web.');

jonahwilliams · 2019-11-25T18:21:09Z

packages/flutter_tools/lib/src/test/flutter_web_platform.dart

+      final num height = body['height'];
+      final Runtime browser = Runtime.chrome;
+      final BrowserManager browserManager = await _browserManagerFor(browser);
+      final ChromeTab chromeTab = await browserManager._browser.chromeConnection.getTab((ChromeTab tab) {


This code should handle the errors thrown by WIP, like WIPError (there might be some more). probably printError and move on if possible

Done, I've handled it by catching WIPError and returning the error to the caller.

jonahwilliams · 2019-11-25T18:22:52Z

packages/flutter_tools/lib/src/test/flutter_web_platform.dart

+/// be executed in a `flutter_tester` environment. This helper class generates a
+/// Dart file configured with flutter_test_config.dart to perform the comparison
+/// of golden files.
+class _TestGoldenComparator {


This is a really interesting approach. i think this PR should have at least one end to end test that goes through this path, if possible.

Added tests for TestGoldenComparator and TestGoldenComparatorProcess, PTAL, thanks.

Piinks · 2019-11-25T18:26:15Z

packages/flutter_goldens_client/lib/skia_client.dart

@@ -371,6 +372,8 @@ class SkiaGoldClient {
    return json.encode(
      <String, dynamic>{
        'Platform' : platform.operatingSystem,
+        if (platform.environment[_kTestBrowserKey] != null)


The documentation above will need to be updated.

packages/flutter/test/widgets/shadow_test.dart

yjbanov

LGTM if LGT @Piinks and @jonahwilliams

packages/flutter_goldens/lib/flutter_goldens.dart

packages/flutter_test/lib/src/matchers.dart

packages/flutter_test/lib/src/_matchers_io.dart

Piinks

Also, this appears to be breaking the current pre-submit workflow, see test failure in the framework-misc shards.

chingjun · 2019-12-02T19:07:06Z

Talked to @Piinks and we are going to wait on #44474, reverted part of my changes in flutter_golden.

Piinks · 2019-12-04T23:57:41Z

#44474 has landed. If you update your branch, you should see the results of pre-submit checks on the Flutter Gold dashboard under Changelists, where you can approve them ahead of time.

chingjun · 2019-12-05T00:02:25Z

Thanks Kate! Just rebased and updated my branch, waiting for the tests to run :)

Piinks · 2019-12-05T00:34:06Z

Not all of the tests have finished yet, but it is looking promising!

Piinks

Curious, does this enable golden file testing for web for everyone?

packages/flutter/test/widgets/shadow_test.dart

chingjun · 2019-12-05T22:22:07Z

Curious, does this enable golden file testing for web for everyone?

Yes. This PR only enabled that for a single test, and we plan to enable all the other tests in a later PR.

chingjun · 2019-12-06T00:52:17Z

The tests now passed with the new images all approved on skia-gold.

@Piinks @jonahwilliams PTAL, thanks.

jonahwilliams · 2019-12-06T01:15:37Z

packages/flutter_tools/lib/src/test/flutter_web_platform.dart

+        });
+        final WipConnection connection = await chromeTab.connect();
+        final WipResponse response = await connection.sendCommand('Page.captureScreenshot', <String, Object>{
+          // Clip the screenshot to include only the element.


Does this actually clip the screenshot in anyway? What if it is offset from the origin?

We are calling window.render() before taking the screenshot (in _matchers_web.dart), so the element is supposed to be at the origin.

https://github.com/flutter/flutter/pull/45530/files#diff-a337fbcec47632d587865bec77ec07ddR88

Can you document that here and where the render is done for posterity?

Added comment here, and in _matches_web.dart as well.

jonahwilliams · 2019-12-06T01:15:56Z

packages/flutter_tools/lib/src/test/flutter_web_platform.dart

+          }
+        });
+        bytes = base64.decode(response.result['data'] as String);
+      } on WipError catch (ex) {


catch FormatException from decoding

jonahwilliams · 2019-12-06T01:22:41Z

packages/flutter_tools/lib/src/test/flutter_web_platform.dart

+
+    final String bootstrap = TestGoldenComparatorProcess.generateBootstrap(testUri);
+    final Process process = await _startProcess(bootstrap);
+    unawaited(_previousComparator?.close());


Does this work in the presence of test concurrency? Or does that not work for golden files?

I noticed that we have --concurrency=1 for web tests on cirrus so I didn't take that into account when writing this.

Do we actually run tests concurrently for web? If so I can try to fix this.

Just talked to @yjbanov , the test runner actually uses the same browser to run multiple tests in multiple iframes, so screenshot taking doesn't work in this case.

jonahwilliams · 2019-12-06T01:23:46Z

packages/flutter_tools/lib/src/test/flutter_web_platform.dart

+          printTrace('<<< $line');
+          return line.isNotEmpty && line[0] == '{';
+        })
+        .map<dynamic>((String line) => jsonDecode(line))


Suggested change

.map<dynamic>((String line) => jsonDecode(line))

.map<dynamic>(jsonDecode)

jonahwilliams · 2019-12-06T01:25:16Z

packages/flutter_tools/lib/src/test/runner.dart

+  // Configure package:test to use the Flutter engine for child processes.
+  final String shellPath = artifacts.getArtifactPath(Artifact.flutterTester);
+  if (!processManager.canRun(shellPath)) {
+    throwToolExit('Cannot find Flutter shell at $shellPath');


Maybe something like "Cannot execute tester at $shellPath"

Done. I actually copied this text from below, in the same file :)

jonahwilliams · 2019-12-06T01:26:27Z

packages/flutter_tools/test/general.shard/web/golden_comparator_test.dart

+      });
+    });
+
+    test('succeed when goldem comparison succeed', () => testbed.run(() async {


Suggested change

test('succeed when goldem comparison succeed', () => testbed.run(() async {

test('succeed when golden comparison succeed', () => testbed.run(() async {

jonahwilliams · 2019-12-06T01:26:52Z

packages/flutter_tools/test/general.shard/web/golden_comparator_test.dart

+      expect(result, null);
+    }));
+
+    test('fail with error message when goldem comparison failed', () => testbed.run(() async {


Suggested change

test('fail with error message when goldem comparison failed', () => testbed.run(() async {

test('fail with error message when golden comparison failed', () => testbed.run(() async {

jonahwilliams · 2019-12-06T01:27:15Z

packages/flutter_tools/test/general.shard/web/golden_comparator_test.dart

+      verify(mockProcessManager.start(any, environment: anyNamed('environment'))).called(1);
+    }));
+
+    test('does not reuse the process for differnt test file', () => testbed.run(() async {


Suggested change

test('does not reuse the process for differnt test file', () => testbed.run(() async {

test('does not reuse the process for different test file', () => testbed.run(() async {

jonahwilliams

Just some small nits, overall LGTM

Piinks

LGTM! 🎉 📸

chingjun · 2019-12-06T22:25:12Z

Thank you everyone who has reviewed and given feedback to this PR!

chingjun added a: tests "flutter test", flutter_test, or one of our tests platform-web Web applications specifically labels Nov 25, 2019

chingjun requested review from yjbanov, jonahwilliams and Piinks November 25, 2019 08:44

fluttergithubbot added framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Nov 25, 2019

googlebot added the cla: yes label Nov 25, 2019

jonahwilliams reviewed Nov 25, 2019

View reviewed changes

Piinks reviewed Nov 25, 2019

View reviewed changes

chingjun force-pushed the flutter-web-screenshot-test branch from 538e2ed to a824f84 Compare November 25, 2019 22:23

yjbanov approved these changes Nov 26, 2019

View reviewed changes

Piinks mentioned this pull request Nov 27, 2019

Flutter Gold is not receiving image uploads #45699

Closed

Piinks reviewed Dec 2, 2019

View reviewed changes

packages/flutter_goldens/lib/flutter_goldens.dart Outdated Show resolved Hide resolved

packages/flutter_test/lib/src/matchers.dart Show resolved Hide resolved

packages/flutter_test/lib/src/_matchers_io.dart Show resolved Hide resolved

Piinks reviewed Dec 2, 2019

View reviewed changes

goderbauer removed the framework flutter/packages/flutter repository. See also f: labels. label Dec 4, 2019

chingjun force-pushed the flutter-web-screenshot-test branch from 8fc7d4c to 96c0ff8 Compare December 5, 2019 00:01

chingjun added 8 commits December 5, 2019 13:15

Implement screenshot test for flutter web.

e0f9ffb

Fixed analyzer errors and some PR comments

5622309

Fix more analyzer errors

41696a5

Add test for TestGoldenComparatorProcess

515db63

Add test for TestGoldenComparator

9ffec8a

Update default platforms, the code expects a list instead of string

4c3223f

Update licenses

f4adba7

Update cirrus to download goldctl

3423904

Update the subprocess to handle all exceptions

77b14eb

chingjun force-pushed the flutter-web-screenshot-test branch 3 times, most recently from 8592f28 to d0c9f64 Compare December 5, 2019 22:06

Piinks reviewed Dec 5, 2019

View reviewed changes

packages/flutter/test/widgets/shadow_test.dart Outdated Show resolved Hide resolved

packages/flutter/test/widgets/shadow_test.dart Outdated Show resolved Hide resolved

packages/flutter/test/widgets/shadow_test.dart Outdated Show resolved Hide resolved

chingjun force-pushed the flutter-web-screenshot-test branch from d0c9f64 to 77b14eb Compare December 5, 2019 22:16

This was referenced Dec 6, 2019

[web] Disabled shadows are not rendered correctly. #46202

Closed

Reset the global variable before running a test. #46203

Merged

jonahwilliams reviewed Dec 6, 2019

View reviewed changes

jonahwilliams approved these changes Dec 6, 2019

View reviewed changes

PR feedbacks

77e44cb

Piinks approved these changes Dec 6, 2019

View reviewed changes

chingjun merged commit c2eb068 into flutter:master Dec 6, 2019

chingjun mentioned this pull request Dec 6, 2019

[web] Add screenshot test capability to flutter/flutter #40297

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021

	throw UnsupportedError('DefaultWebGoldenComparator is noonly supported on the web.');
	throw UnsupportedError('DefaultWebGoldenComparator is not supported on the web.');

	.map<dynamic>((String line) => jsonDecode(line))
	.map<dynamic>(jsonDecode)

	test('succeed when goldem comparison succeed', () => testbed.run(() async {
	test('succeed when golden comparison succeed', () => testbed.run(() async {

	test('fail with error message when goldem comparison failed', () => testbed.run(() async {
	test('fail with error message when golden comparison failed', () => testbed.run(() async {

	test('does not reuse the process for differnt test file', () => testbed.run(() async {
	test('does not reuse the process for different test file', () => testbed.run(() async {

Implement screenshot test for flutter web. #45530

Implement screenshot test for flutter web. #45530

Conversation

chingjun commented Nov 25, 2019

Description

Related Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjbanov left a comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

chingjun commented Dec 2, 2019

Piinks commented Dec 4, 2019

chingjun commented Dec 5, 2019

Piinks commented Dec 5, 2019

Piinks left a comment

Choose a reason for hiding this comment

chingjun commented Dec 5, 2019

chingjun commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

chingjun commented Dec 6, 2019