Improve WebDriver error message #81107

jushutch · 2021-04-24T04:24:30Z

The current WebDriver error message is too vague, since it's a generic message with no specific guidance for each browser. Adding a link to the documentation on WebDrivers to help troubleshooting. This same pattern exists elsewhere in the code and works well in directing people to the right information for resolving the error.

An example of the new message including the link:

Make sure you have the correct WebDriver Server running at 4444. 
Make sure the WebDriver Server matches option --browser-name. 
For more information see: https://flutter.dev/docs/testing/integration-tests#running-in-a-browser 
SocketException: OS Error: The remote computer refused the network connection.
, errno = 1225, address = localhost, port = 61383

This PR fixes issue #78472.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

flutter-dashboard · 2021-04-24T04:24:33Z

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.

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.

jonahwilliams · 2021-04-25T17:27:31Z

It seems reasonable that this could be test exempt but I would ask in the discord according to the instructions above.

jonahwilliams · 2021-04-29T20:41:49Z

@Hixie did this get marked as test exempt or not?

Hixie · 2021-04-29T21:05:13Z

It did not (test-exempt PRs have a comment somewhere that starts with test-exempt: ). I would recommend having a test, in part because hopefully a test would catch the error in the PR. :-)

The current WebDriver message is incomplete. Add link to the documentation on WebDrivers for different browsers to help troubleshooting.

Add a simple test to verify that the WebDriver error message includes a link to the documentation.

jushutch · 2021-04-30T03:14:11Z

I just added a simple test to check that the error message contains the link that was added. However I must be missing the error in the PR that was mentioned; @Hixie would you mind elaborating to save some time? Is there more that I should be explicitly testing for in this change? Thanks for the help!

packages/flutter_tools/test/general.shard/drive/drive_service_test.dart

Hixie · 2021-04-30T06:26:04Z

packages/flutter_tools/lib/src/drive/web_driver_service.dart

@@ -111,6 +111,8 @@ class WebDriverService extends DriverService {
        'Unable to start WebDriver Session for Flutter for Web testing. \n'
        'Make sure you have the correct WebDriver Server running at $driverPort. \n'
        'Make sure the WebDriver Server matches option --browser-name. \n'
+        'For more information see: '
+        'https://flutter.dev/docs/testing/integration-tests#running-in-a-browser \n'


the trailing space before the newline here is what i was referring to

Okay, thanks for pointing it out. I assumed this was the correct style based on the existing message, but let me know if you prefer otherwise and I can remove the trailing spaces for the entire message.

Friendly ping @Hixie

The existing text also needs fixing, I hadn't noticed that.

There shouldn't be a trailing space before a newline.

I removed all of the trailing spaces before new lines in this message 👍

The previous implementation was vulnerable to false positives when a ToolExit exception isn't thrown. Change to fail when no ToolExit is thrown.

Specify types to meet style rules.

jonahwilliams

LGTM if lg to @Hixie

Hixie · 2021-05-18T22:05:34Z

packages/flutter_tools/test/general.shard/drive/drive_service_test.dart

+      <String, String>{},
+      PackageConfig(<Package>[Package('test', Uri.base)]),
+      browserName: 'chrome',
+    ), throwsToolExit(message: RegExp(' $link ')));


I would probably make this test more strict. It's not just that there's a link that matters, it's also that the text is correct around the link, right?

Good point, I made the test more strict by including the preamble 👍

Remove trailing spaces before newlines to follow style guide.

Test that the link in the error message includes an appropriate preamble and follows style rules.

jonahwilliams · 2021-05-27T20:35:52Z

This branch unfortunately has merge conflicts that need to be handled before we could land it.

jonahwilliams · 2021-06-10T15:52:27Z

I don't know why this got stuck so I'm merging it

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 24, 2021

google-cla bot added the cla: yes label Apr 24, 2021

jushutch added 2 commits April 29, 2021 23:08

Add link to documentation in error message

1565889

The current WebDriver message is incomplete. Add link to the documentation on WebDrivers for different browsers to help troubleshooting.

Add test for documentation link

a480ed9

Add a simple test to verify that the WebDriver error message includes a link to the documentation.

jushutch force-pushed the 78472-webdriver-error-message branch from bd9ef65 to a480ed9 Compare April 30, 2021 03:10

jonahwilliams reviewed Apr 30, 2021

View reviewed changes

packages/flutter_tools/test/general.shard/drive/drive_service_test.dart Outdated Show resolved Hide resolved

Hixie reviewed Apr 30, 2021

View reviewed changes

jushutch added 2 commits May 1, 2021 17:27

Expect ToolExit exception in test

33ecc01

The previous implementation was vulnerable to false positives when a ToolExit exception isn't thrown. Change to fail when no ToolExit is thrown.

Add type annotations to test

9610c5b

Specify types to meet style rules.

jonahwilliams approved these changes May 11, 2021

View reviewed changes

Hixie reviewed May 18, 2021

View reviewed changes

jushutch added 2 commits May 22, 2021 13:19

Remove spaces before newlines

4371c94

Remove trailing spaces before newlines to follow style guide.

Make test for error message more strict

ec7e91c

Test that the link in the error message includes an appropriate preamble and follows style rules.

Merge branch 'master' into 78472-webdriver-error-message

6f88275

jonahwilliams added the waiting for tree to go green label Jun 3, 2021

jonahwilliams merged commit db528a2 into flutter:master Jun 10, 2021

Improve WebDriver error message #81107

Improve WebDriver error message #81107

Uh oh!

Conversation

jushutch commented Apr 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Apr 24, 2021

Uh oh!

jonahwilliams commented Apr 25, 2021

Uh oh!

jonahwilliams commented Apr 29, 2021

Uh oh!

Hixie commented Apr 29, 2021

Uh oh!

jushutch commented Apr 30, 2021

Uh oh!

Uh oh!

Hixie Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

jushutch Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

jonahwilliams May 11, 2021

Choose a reason for hiding this comment

Uh oh!

Hixie May 18, 2021

Choose a reason for hiding this comment

Uh oh!

jushutch May 29, 2021

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Hixie May 18, 2021

Choose a reason for hiding this comment

Uh oh!

jushutch May 29, 2021

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented May 27, 2021

Uh oh!

jonahwilliams commented Jun 10, 2021

Uh oh!

Uh oh!

jushutch commented Apr 24, 2021 •

edited

Loading