-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Use Async WebDriver for WebFlutterDriver. #50835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks a lot! This is great! |
|
FYI @zanderso |
zanderso
left a comment
There was a problem hiding this 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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetize imports.
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to throw.
| rethrow; | ||
| } | ||
| throwToolExit('CAUGHT EXCEPTION: $error\n$stackTrace'); | ||
| throw 'Unable to run test: $error\n$stackTrace'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
''');
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
jonahwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
chromedriver --port=4444instead ofjava 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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.