Skip to content

Conversation

@angjieli
Copy link
Contributor

Description

  • Switch to Async WebDriver for WebFlutterDriver.
  • Provides more meaningful help text when unable to create WebDriver session.
  • No longer needs Java/Selenium Server. WebDriver Binaries are still needed. This means you can run chromedriver --port=4444 instead of java jar selenium-server-standalone-3.141.59.jar.

Related Issues

#50144
#50146

Tests

I added the following tests:

Fixed test for drive_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@fluttergithubbot fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Feb 14, 2020
@nturgut
Copy link
Contributor

nturgut commented Feb 14, 2020

Thanks a lot! This is great!
/cc @yjbanov

@jonahwilliams
Copy link
Contributor

FYI @zanderso

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DBC


if (isWebPlatform) {
throwToolExit(
'Flutter Driver (web) does not support running without use-existing-app.\n'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 2 space indent on a continued line. Here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

import 'dart:math' as math;

import 'package:webdriver/sync_io.dart' as sync_io;
import 'package:webdriver/async_io.dart' as async_io;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetize imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (error is ToolExit) {
rethrow;
}
throwToolExit('CAUGHT EXCEPTION: $error\n$stackTrace');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tool exits should give user-actionable information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to throw.

@nturgut nturgut mentioned this pull request Feb 19, 2020
13 tasks
rethrow;
}
throwToolExit('CAUGHT EXCEPTION: $error\n$stackTrace');
throw 'Unable to run test: $error\n$stackTrace';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this throw an exception or error type instead of a String

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

throw 'Unable to run test: $error\n$stackTrace';
} finally {
driver?.quit();
await driver?.quit();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this where the 15 second timeout happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. The test process will take 15 seconds before exiting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg, thanks

y = int.parse(dimensions[1]);
} on FormatException catch (ex) {
throw FormatException('''
Dimension provided is invalid.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a tool exit. If you use ''' then you will pick up all the whitespace to the left, instead format it like:

        throwToolExit('''
Dimension provided to --browser-dimension is invalid:
$ex
''');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

browser,
argResults['headless'].toString() == 'true',
);
} catch (ex) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on Exception or other specific type. As is, this will catch NoSuchMethodError and TypeError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fluttergithubbot fluttergithubbot merged commit 11549e4 into flutter:master Feb 19, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants