Implement Webdriver-based Safari test harness #33757

eyebrowsoffire · 2022-06-01T18:42:20Z

This adds support for Webdriver-based browser tests and changes the macos Safari implementation to use this method. There is a fair amount of refactoring to the Browser and BrowserEnvironment classes and the test platform that uses them.

This is part of a fix for flutter/flutter#99885. We still need to make some changes to the LUCI recipe repo in order to fix the rest.

flutter-dashboard · 2022-06-01T18:42:23Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

This reverts commit 1d02dc7.

skia-gold · 2022-06-03T22:40:50Z

Gold has detected about 129 new digest(s) on patchset 6.
View them at https://flutter-engine-gold.skia.org/cl/github/33757

flutter-dashboard · 2022-06-03T22:52:57Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #33757 at sha 7c70e2e

skia-gold · 2022-06-04T00:45:51Z

Gold has detected about 112 new digest(s) on patchset 7.
View them at https://flutter-engine-gold.skia.org/cl/github/33757

flutter-dashboard · 2022-06-06T19:00:16Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #33757 at sha efc8011

skia-gold · 2022-06-06T20:40:50Z

Gold has detected about 218 new digest(s) on patchset 8.
View them at https://flutter-engine-gold.skia.org/cl/github/33757

skia-gold · 2022-06-07T17:20:50Z

Gold has detected about 87 new digest(s) on patchset 9.
View them at https://flutter-engine-gold.skia.org/cl/github/33757

mdebbar

Overall, looks good to me with some minor nits and comments.

mdebbar · 2022-06-07T20:12:01Z

lib/web_ui/dev/safari_ios.dart

    if (!file.existsSync()) {
      throw Exception('Failed to read the screenshot '
-          'screenshot$_fileNameCounter.png.');
+          'screenshot$screenshotTag.png.');


nit:

Suggested change

'screenshot$screenshotTag.png.');

'$filename.');

mdebbar · 2022-06-08T14:57:58Z

lib/web_ui/dev/test_platform.dart

    final String filename = requestData['filename'] as String;

-    if (_screenshotManager == null) {
+    if (!(await _browserManager!)!.supportsScreenshots) {


I'm trying to think of a way to avoid the double ! in multiple places. How about adding something like:

Future<BrowserManager> get browserManager async { return (await _browserManager!)!; }

then in your if (and in the other place below):

if (!(await browserManager).supportsScreenshots) {

I'm not sure how much improvement that makes. I feel like the main problem here is that _browserManager is a future that has to be awaited everywhere. Ideally, _browserManager should simply be an instance of BrowserManager (or BrowserManager?). The future should be awaited and resolved somewhere else before any of this code runs.

But that's outside the scope of this PR, so I digress.

I agree with you and I actually spent some time bikeshedding and trying to get rid of some of the layers of optionality/futures here. However, I ended up backing out those changes because their scope was creeping outside of really how far I wanted to go with this change. A convenience method like you suggest would be simple enough to add though, good suggestion.

mdebbar · 2022-06-08T15:01:06Z

lib/web_ui/dev/webdriver_browser.dart

+        .transform(utf8.decoder)
+        .transform(const LineSplitter())
+        .listen((String error) {
+      print('[Webdriver] $error');


Maybe add "[Error]" so it's easier to distinguish errors from normal logs?

Suggested change

print('[Webdriver] $error');

print('[Webdriver][Error] $error');

mdebbar · 2022-06-08T15:06:02Z

lib/web_ui/dev/webdriver_browser.dart

+    for(;;) {
+      try {
+        final WebDriver driver = await createDriver(
+          uri: driverUri, desired: <String, dynamic>{'browserName': packageTestRuntime.identifier});
+        return WebDriverBrowser(driver, url);
+      } on SocketException {
+        // Sometimes we may try to connect before the web driver port is ready. Retry.
+        print('Failed to connect to webdriver process. Retrying in 100 ms');
+        await Future<void>.delayed(const Duration(milliseconds: 100));
+      } catch (exception) {
+        print('Exception while creating webdriver: $exception');
+        rethrow;
+      }
+    }


This could become a real infinite loop (e.g. if webdriver never starts for whatever reason). Since there's no timeout or max retry logic here, are you relying on the test suite's timeout to stop this loop? If that's the case, it would be nice to clarify that in a comment.

Yes, I'll add a comment.

skia-gold · 2022-06-08T18:55:50Z

Gold has detected about 148 new digest(s) on patchset 10.
View them at https://flutter-engine-gold.skia.org/cl/github/33757

yjbanov

A handful of nitpicks. LGTM overall.

yjbanov · 2022-06-09T16:14:09Z

lib/web_ui/dev/browser.dart

  /// browser instances.
  Future<void> prepare();

+  // Perform any necessary teardown steps


nit: use /// for doc comments

yjbanov · 2022-06-09T16:18:42Z

lib/web_ui/dev/webdriver_browser.dart

+
+  @override
+  Future<Browser> launchBrowserInstance(Uri url, {bool debug = false}) async {
+    for (;;) {


nit: I think while (true) is more idiomatic in Dart

yjbanov · 2022-06-09T16:19:38Z

lib/web_ui/dev/webdriver_browser.dart

+        return WebDriverBrowser(driver, url);
+      } on SocketException {
+        // Sometimes we may try to connect before the web driver port is ready.
+        // So we should here. Note that if there was some issue with the


nit: missing word between "should" and "here"

yjbanov · 2022-06-09T16:20:41Z

lib/web_ui/dev/webdriver_browser.dart

+        print('Failed to connect to webdriver process. Retrying in 100 ms');
+        await Future<void>.delayed(const Duration(milliseconds: 100));
+      } catch (exception) {
+        print('Exception while creating webdriver: $exception');


When rethrowing you don't need to also print, because the rethrow will retain all the information about the original error, including its stack trace.

yjbanov · 2022-06-09T16:23:06Z

lib/web_ui/dev/webdriver_browser.dart

+        final WebDriver driver = await createDriver(
+          uri: driverUri, desired: <String, dynamic>{'browserName': packageTestRuntime.identifier});
+        return WebDriverBrowser(driver, url);
+      } on SocketException {


I wonder if it would be more appropriate to smoke test (and retry if necessary) the driver process in the prepare method. This way other methods that need the driver to be ready don't need to mistrust the initialization process of the driver.

I tried coming up with a good solution here based on your suggestion, but I'm not really sure I really like anything I could come up with. I understand where you're coming from in that it seems like the responsibility of "prepare" to ensure that the web driver process is actually ready to go, but I think actually trying to enforce this only ends up with negatives practically speaking. For one thing, doing the smoke test in prepare results in opening an extra socket just to check if the driver process is ready, which is a non-trivial operation from a perf perspective. In practice, this is being validated the first time launchBrowserInstance is invoked, and after that, other browser launches should end up in the happy path. The only real benefit I could see to doing the validation in prepare is that if there is some situation where the web driver launches and the smoke test passes, but something bad happens in the web driver process before a subsequent invocation of launchBrowserInstance then we would fail fast rather than have to wait for the test timeout. But this seems like an edge case to optimize for that (hopefully) wouldn't happen too often in practice anyway.

yjbanov · 2022-06-09T16:24:42Z

lib/web_ui/dev/webdriver_browser.dart

+  Uri get driverUri;
+
+  @override
+  Future<void> prepare() async {


Sometimes there's an existing driver process hanging around on the system. It may be useful to check that the port is not already taken. Otherwise, the test might end up using the wrong driver process (depending on when exactly the newly spawned process crashes due to the taken port).

I found a nice simple implementation of a "find unused port" function in the dart SDK, which I am shamefully stealing here. That should solve this problem.

yjbanov · 2022-06-09T16:25:50Z

lib/web_ui/dev/webdriver_browser.dart

+  }
+}
+
+class WebDriverBrowser extends Browser {


I love how simple the driver-based implementation is!

…#33757)

* WebDriver stuff. * Some refactoring and WebDriver improvements. * Add license comments. * TODO needs to have two slashes, not three. * Remove dependency on OS/Xcode * Revert "Remove dependency on OS/Xcode" This reverts commit 1d02dc7. * Disable golden tests for macOS Safari that were disabled for iOS Safari. * Don't run tests that didn't run on ios Safari on Desktop Safari. * Newline at end of file. * Fixes as per Mouad's comments. * Changes as per Yegor's comments. * Fix analyzer issue.

eyebrowsoffire added 2 commits May 31, 2022 10:13

WebDriver stuff.

7b10b5b

Some refactoring and WebDriver improvements.

19a0498

flutter-dashboard bot added the platform-web Code specifically for the web engine label Jun 1, 2022

flutter-dashboard bot added the needs tests label Jun 1, 2022

eyebrowsoffire added 4 commits June 1, 2022 12:46

Add license comments.

4fea3c9

TODO needs to have two slashes, not three.

23ffcbb

Remove dependency on OS/Xcode

1d02dc7

Revert "Remove dependency on OS/Xcode"

7c70e2e

This reverts commit 1d02dc7.

flutter-dashboard bot added the will affect goldens label Jun 3, 2022

Disable golden tests for macOS Safari that were disabled for iOS Safari.

efc8011

Don't run tests that didn't run on ios Safari on Desktop Safari.

44052ee

eyebrowsoffire mentioned this pull request Jun 6, 2022

Golden tests using Safaridriver take screenshots that visually differ from the browser window flutter/flutter#105473

Closed

eyebrowsoffire requested review from mdebbar and yjbanov June 7, 2022 17:00

Newline at end of file.

e67ab11

eyebrowsoffire mentioned this pull request Jun 7, 2022

Mac Web Engine timing out on Safari on Xcode 13.* flutter/flutter#99885

Closed

mdebbar approved these changes Jun 8, 2022

View reviewed changes

Fixes as per Mouad's comments.

51767c6

Merge branch 'main' into webdriver

e357bd1

yjbanov approved these changes Jun 9, 2022

View reviewed changes

eyebrowsoffire added 2 commits June 9, 2022 10:05

Changes as per Yegor's comments.

c282638

Fix analyzer issue.

98f37fd

eyebrowsoffire merged commit fc8cdcf into flutter:main Jun 9, 2022

eyebrowsoffire mentioned this pull request Jun 9, 2022

Update web_engine Flutter Recipe to not rely on Xcode/iOS Simulator flutter/flutter#105721

Closed

engine-flutter-autoroll mentioned this pull request Jun 9, 2022

Roll Flutter Engine from 9ca483101b6d to fc8cdcf4e9dd (4 revisions) flutter/flutter#105726

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 9, 2022

fc8cdcf Implement Webdriver-based Safari test harness (flutter/engine…

1f14d15

…#33757)

eyebrowsoffire deleted the webdriver branch June 11, 2022 00:01

	print('[Webdriver] $error');
	print('[Webdriver][Error] $error');

Implement Webdriver-based Safari test harness #33757

Implement Webdriver-based Safari test harness #33757

Uh oh!

Conversation

eyebrowsoffire commented Jun 1, 2022

Uh oh!

flutter-dashboard bot commented Jun 1, 2022

Uh oh!

skia-gold commented Jun 3, 2022

Uh oh!

flutter-dashboard bot commented Jun 3, 2022

Uh oh!

skia-gold commented Jun 4, 2022

Uh oh!

flutter-dashboard bot commented Jun 6, 2022

Uh oh!

skia-gold commented Jun 6, 2022

Uh oh!

skia-gold commented Jun 7, 2022

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skia-gold commented Jun 8, 2022

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants