Add integration tests for the execution domain.

Add integration tests for the execution domain.

BUG=
R=brianwilkerson@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/4667e59e85d19496ca3763f7334e94e396ab8be8

[devoncarew, 2017-02-12 03:04:27]

devoncarew@google.com changed reviewers:
+ brianwilkerson@google.com, paulberry@google.com, scheglov@google.com

[devoncarew, 2017-02-12 03:04:28]

- add integration tests for execution.createContext and execution.deleteContext
- update the coverage.md file
- remove a TODO from Paul; when running the tests an error result was returning
an UnimplementedError; instead we now pass the original error through the
returned Future

I made matching changes in analysis_server integration_tests.dart and
analyzer_plugin integration_tests.dart; it does seem like this (fairly large)
file exists in two places.

[scheglov, 2017-02-12 03:18:25]

lgtm

[Paul Berry, 2017-02-12 14:39:04]

https://codereview.chromium.org/2691763002/diff/1/pkg/analysis_server/test/in...
File pkg/analysis_server/test/integration/execution/delete_context_test.dart
(right):

https://codereview.chromium.org/2691763002/diff/1/pkg/analysis_server/test/in...
pkg/analysis_server/test/integration/execution/delete_context_test.dart:39: }
catch (ignore) {}
This won't work because the way the fail() method reports test failures is to
throw an exception, so the fact that you're catching *all* exceptions means that
even if MapUri doesn't report the expected error, the test will still pass.  We
don't want that.

See my comments in pkg/analysis_server/test/integration/integration_tests.dart. 
Assuming you take my advice there, this could change to:

} on ServerErrorMessage catch(message) {
  expect(message.error['code'], 'INVALID_PARAMETER');
}

https://codereview.chromium.org/2691763002/diff/1/pkg/analysis_server/test/in...
File pkg/analysis_server/test/integration/integration_tests.dart (right):

https://codereview.chromium.org/2691763002/diff/1/pkg/analysis_server/test/in...
pkg/analysis_server/test/integration/integration_tests.dart:580:
completer.completeError(messageAsMap['error']);
I'd recommend making a new class which represents an error received from the
server, and contains the error message JSON as a field (perhaps call the class
"ServerErrorMessage").  That way we can catch the specific exception type
without accidentally catching other exceptions.

https://codereview.chromium.org/2691763002/diff/1/pkg/analyzer_plugin/test/in...
File pkg/analyzer_plugin/test/integration/support/integration_tests.dart
(right):

https://codereview.chromium.org/2691763002/diff/1/pkg/analyzer_plugin/test/in...
pkg/analyzer_plugin/test/integration/support/integration_tests.dart:561:
completer.completeError(messageAsMap['error']);
Hmm, when did this code get duplicated?  We should probably de-dup these files.

(Not necessary to fix for this CL; just thought I would mention it in case
anyone remembers the reason for the code duplication).

[Brian Wilkerson, 2017-02-12 16:41:54]

lgtm, modulo Paul's comments

https://codereview.chromium.org/2691763002/diff/1/pkg/analyzer_plugin/test/in...
File pkg/analyzer_plugin/test/integration/support/integration_tests.dart
(right):

https://codereview.chromium.org/2691763002/diff/1/pkg/analyzer_plugin/test/in...
pkg/analyzer_plugin/test/integration/support/integration_tests.dart:561:
completer.completeError(messageAsMap['error']);
> Hmm, when did this code get duplicated?  We should probably de-dup these
files.

It was duplicated when I created the analyzer_plugin package.

As unfortunate as the duplication is, we can't avoid all of the duplication
because this file is referencing classes from the analyzer_plugin's protocol and
the other file is referencing classes from the analysis_server's protocol, and
the two sets of classes are distinct. It might be worth unifying portions of
them at some point, but that depends on how much could be unified.

In the interest of full disclosure, there are other libraries that were also
duplicated and I'm much more interested in deduplicating those at some point.
But I'll note that the shape of the plugin protocol is still changing and I
would prefer to finish defining it before attempting any deduplication.

[devoncarew, 2017-02-12 17:52:35]

Updated! ptal

https://codereview.chromium.org/2691763002/diff/1/pkg/analysis_server/test/in...
File pkg/analysis_server/test/integration/execution/delete_context_test.dart
(right):

https://codereview.chromium.org/2691763002/diff/1/pkg/analysis_server/test/in...
pkg/analysis_server/test/integration/execution/delete_context_test.dart:39: }
catch (ignore) {}
On 2017/02/12 14:39:04, Paul Berry wrote:
> This won't work because the way the fail() method reports test failures is to
> throw an exception, so the fact that you're catching *all* exceptions means
that
> even if MapUri doesn't report the expected error, the test will still pass. 
We
> don't want that.
> 
> See my comments in
pkg/analysis_server/test/integration/integration_tests.dart. 
> Assuming you take my advice there, this could change to:
> 
> } on ServerErrorMessage catch(message) {
>   expect(message.error['code'], 'INVALID_PARAMETER');
> }

Ah, good catch! I'll add a class as above.

https://codereview.chromium.org/2691763002/diff/1/pkg/analysis_server/test/in...
File pkg/analysis_server/test/integration/integration_tests.dart (right):

https://codereview.chromium.org/2691763002/diff/1/pkg/analysis_server/test/in...
pkg/analysis_server/test/integration/integration_tests.dart:580:
completer.completeError(messageAsMap['error']);
On 2017/02/12 14:39:04, Paul Berry wrote:
> I'd recommend making a new class which represents an error received from the
> server, and contains the error message JSON as a field (perhaps call the class
> "ServerErrorMessage").  That way we can catch the specific exception type
> without accidentally catching other exceptions.

Done.

https://codereview.chromium.org/2691763002/diff/1/pkg/analyzer_plugin/test/in...
File pkg/analyzer_plugin/test/integration/support/integration_tests.dart
(right):

https://codereview.chromium.org/2691763002/diff/1/pkg/analyzer_plugin/test/in...
pkg/analyzer_plugin/test/integration/support/integration_tests.dart:561:
completer.completeError(messageAsMap['error']);
On 2017/02/12 16:41:54, Brian Wilkerson wrote:
> > Hmm, when did this code get duplicated?  We should probably de-dup these
> files.
> 
> It was duplicated when I created the analyzer_plugin package.
> 
> As unfortunate as the duplication is, we can't avoid all of the duplication
> because this file is referencing classes from the analyzer_plugin's protocol
and
> the other file is referencing classes from the analysis_server's protocol, and
> the two sets of classes are distinct. It might be worth unifying portions of
> them at some point, but that depends on how much could be unified.
> 
> In the interest of full disclosure, there are other libraries that were also
> duplicated and I'm much more interested in deduplicating those at some point.
> But I'll note that the shape of the plugin protocol is still changing and I
> would prefer to finish defining it before attempting any deduplication.

Is it important to keep this files in sync? If so, perhaps we should add some
comments to that effect at the top of both files.

[Brian Wilkerson, 2017-02-12 18:47:09]

https://codereview.chromium.org/2691763002/diff/1/pkg/analyzer_plugin/test/in...
File pkg/analyzer_plugin/test/integration/support/integration_tests.dart
(right):

https://codereview.chromium.org/2691763002/diff/1/pkg/analyzer_plugin/test/in...
pkg/analyzer_plugin/test/integration/support/integration_tests.dart:561:
completer.completeError(messageAsMap['error']);
> Is it important to keep this files in sync?

That depends on what you mean by "important". :-)

It is, of course, a good engineering practice to avoid duplicating code when
possible, but the files are dealing with two separate, albeit similar,
protocols. It isn't a requirement that the protocols remain the same over time
(and they are already different in minor ways), so while I wouldn't say it's
critical, and I think it's less important than getting the plugin support
finished, I think it is important on some level.

https://codereview.chromium.org/2691763002/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/integration/integration_tests.dart (right):

https://codereview.chromium.org/2691763002/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/integration/integration_tests.dart:283: * An error
result from a server call.
nit: "call" --> "request"

https://codereview.chromium.org/2691763002/diff/20001/pkg/analyzer_plugin/tes...
File pkg/analyzer_plugin/test/integration/support/integration_tests.dart
(right):

https://codereview.chromium.org/2691763002/diff/20001/pkg/analyzer_plugin/tes...
pkg/analyzer_plugin/test/integration/support/integration_tests.dart:264: * An
error result from a server call.
I was going to suggest changing "server" to "plugin" (and rename the class to
"PluginErrorMessage"), but now that I look at the code more closely, I'm not
sure that's appropriate. My guess at this point (I can look into this more if
you want me to) is that I copied too much when copying this file and that a
large portion of this file is probably unused. (Sure would be nice to have a
global dead-code analysis tool.)

nit: "call" --> "request"

[devoncarew, 2017-02-12 19:22:48]

https://codereview.chromium.org/2691763002/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/integration/integration_tests.dart (right):

https://codereview.chromium.org/2691763002/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/integration/integration_tests.dart:283: * An error
result from a server call.
On 2017/02/12 18:47:09, Brian Wilkerson wrote:
> nit: "call" --> "request"

Done.

https://codereview.chromium.org/2691763002/diff/20001/pkg/analyzer_plugin/tes...
File pkg/analyzer_plugin/test/integration/support/integration_tests.dart
(right):

https://codereview.chromium.org/2691763002/diff/20001/pkg/analyzer_plugin/tes...
pkg/analyzer_plugin/test/integration/support/integration_tests.dart:264: * An
error result from a server call.
On 2017/02/12 18:47:09, Brian Wilkerson wrote:
> I was going to suggest changing "server" to "plugin" (and rename the class to
> "PluginErrorMessage"), but now that I look at the code more closely, I'm not
> sure that's appropriate. My guess at this point (I can look into this more if
> you want me to) is that I copied too much when copying this file and that a
> large portion of this file is probably unused. (Sure would be nice to have a
> global dead-code analysis tool.)
> 
> nit: "call" --> "request"

Updated the dartdoc (call -> request).

I'm happy to take a quick look through this file (after this CL lands).

[devoncarew, 2017-02-12 19:23:38]

Description was changed from

==========
Add integration tests for the execution domain.

BUG=
==========

to

==========
Add integration tests for the execution domain.

BUG=
R=brianwilkerson@google.com, scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4667e59e85d19496ca3763f7334e94e396ab8be8
==========

[devoncarew, 2017-02-12 19:23:39]

Committed patchset #3 (id:40001) manually as
4667e59e85d19496ca3763f7334e94e396ab8be8 (presubmit successful).