Skip to content

Conversation

@DanTup
Copy link
Contributor

@DanTup DanTup commented May 1, 2019

Fixes #617.

The VM service only allows us to return valid results with a type field, or errors. Anything else and it unregisters the service. This adds a catch-all catch block to ensure we return a well-formed error in the case of any exception (which includes the error + stack).

In VS Code, we'll catch this and show the user a nicer message, with a button to show the full error/stack for reporting (most likely it'll be that the Chrome executable wasn't found).

@DanTup
Copy link
Contributor Author

DanTup commented May 1, 2019

Note if reviewing: because I wrapped this in a try/catch, the indenting changed, which resulted in some lines wrapping, which results in a messy diff (!). If you tick the "Ignore whitespace" option in the diff settings, it'll be much easier to see the actual changes :-)

@DanTup
Copy link
Contributor Author

DanTup commented May 1, 2019

Test failure appears to be an unrelated flake, kicked it and opened #622.


return {'result': Success().toJson()};
} catch (e, s) {
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment explaining why this exact format is used.

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

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

Go ahead and rev the server version and publish this package.
lgtm

@DanTup
Copy link
Contributor Author

DanTup commented May 1, 2019

Hmm, looks like another flake. This change is only in the server and isn't even used by these tests (we have to explicitly rev the version in pubspec) so I'm landing this in the interests of progressing. I'll ensure the build is green before publishing the server.

02:13 +17 -1: test/integration_tests/integration_test.dart: integration debugging console output [E]
  Expected: contains '1\n'
              ''
    Actual: '\n'
              ''
  
  package:test_api                            expect
  test/integration_tests/debugger.dart 288:5  debuggingTests.<fn>
  ===== asynchronous gap ===========================
  dart:async                                  _AsyncAwaitCompleter.completeError
  test/integration_tests/debugger.dart        debuggingTests.<fn>
  ===== asynchronous gap ===========================
  dart:async                                  _asyncThenWrapperHelper
  test/integration_tests/debugger.dart        debuggingTests.<fn>

@DanTup DanTup merged commit d75e017 into flutter:master May 1, 2019
@DanTup DanTup deleted the launch-error branch May 1, 2019 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve failure of launchDevTools when Chrome is not installed

2 participants