Create an apptesting framework for dart.

Create an apptesting framework for dart.

This directly imports the dart unittest/unittest library and
dependencies into third_party/, and creates a new apptest.dartzip which
can be linked into your packaged mojo dart applications.

This patch also rebuilds the current C++ apptests which test dart code
in dart to show feasibility.

BUG=459376
R=msw@chromium.org, tonyg@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/373b74f1339c7953007662dd5237d8507e156655

[Elliot Glaysher, 2015-03-02 22:37:16]

erg@chromium.org changed reviewers:
+ tonyg@chromium.org, zra@google.com

[Elliot Glaysher, 2015-03-02 22:37:17]

zra: first round review; still working on porting the last few tests, but I
wanted to get an opinion on the general structure here.

tonyg: fyi. I'm not sure how much longer this will take, but I want to see if
you think this is a good layout for tests.

https://codereview.chromium.org/971083002/diff/1/services/dart/dart_apptests/...
File services/dart/dart_apptests/pingpong_apptests.dart (right):

https://codereview.chromium.org/971083002/diff/1/services/dart/dart_apptests/...
services/dart/dart_apptests/pingpong_apptests.dart:68: });
This is incomplete and I'm currently working on the rest of these tests.

[tonyg, 2015-03-02 22:43:31]

https://codereview.chromium.org/971083002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/971083002/diff/1/BUILD.gn#newcode16
BUILD.gn:16: "//third_party/dart-packages/unittest",
nit: alphabetize

https://codereview.chromium.org/971083002/diff/1/mojo/dart/apptest/BUILD.gn
File mojo/dart/apptest/BUILD.gn (right):

https://codereview.chromium.org/971083002/diff/1/mojo/dart/apptest/BUILD.gn#n...
mojo/dart/apptest/BUILD.gn:7: dart_package("apptest") {
Are we planning to make this public?

https://codereview.chromium.org/971083002/diff/1/third_party/dart-packages/ma...
File third_party/dart-packages/matcher/BUILD.gn (right):

https://codereview.chromium.org/971083002/diff/1/third_party/dart-packages/ma...
third_party/dart-packages/matcher/BUILD.gn:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
These BUILD.gn files should be in mirror directories in build/secondary, right?
Then eventually these directories could be pulled in via DEPS.

Also, these packages also exist in sky/tests/resources/third_party. When this
lands, we should make sure that they're only in one place in the repo.

[Elliot Glaysher, 2015-03-02 23:33:37]

Finished porting the C++ tests and fixed some problems with the gn files.

https://codereview.chromium.org/971083002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/971083002/diff/1/BUILD.gn#newcode16
BUILD.gn:16: "//third_party/dart-packages/unittest",
On 2015/03/02 22:43:31, tonyg wrote:
> nit: alphabetize

Removed. (It was indicative of a deeper problem with the gn files that I wrote.)

https://codereview.chromium.org/971083002/diff/1/mojo/dart/apptest/BUILD.gn
File mojo/dart/apptest/BUILD.gn (right):

https://codereview.chromium.org/971083002/diff/1/mojo/dart/apptest/BUILD.gn#n...
mojo/dart/apptest/BUILD.gn:7: dart_package("apptest") {
On 2015/03/02 22:43:31, tonyg wrote:
> Are we planning to make this public?

Yes. I assumed that this dartzip would be a product that could be used in other
repos. Is there something further that I have to do around this?

https://codereview.chromium.org/971083002/diff/20001/services/dart/dart_appte...
File services/dart/dart_apptests/pingpong_apptests.dart (right):

https://codereview.chromium.org/971083002/diff/20001/services/dart/dart_appte...
services/dart/dart_apptests/pingpong_apptests.dart:83: // tests, which is weird.
zra: I'd like some guidance here. What further do I have to do to make make the
next line work in the current version of the bindings? Is .impl the wrong thing
to pass here?

[Elliot Glaysher, 2015-03-02 23:40:49]

https://codereview.chromium.org/971083002/diff/20001/services/dart/dart_appte...
File services/dart/dart_apptests/pingpong_apptests.dart (right):

https://codereview.chromium.org/971083002/diff/20001/services/dart/dart_appte...
services/dart/dart_apptests/pingpong_apptests.dart:115:
pingPongServiceProxy.close();
There's also a deeper question about if we can make things work without explicit
close() calls. If one of these things throws and unwinds the stack before
calling close on all these thing, I assume we just hang the program.

[tonyg, 2015-03-03 00:07:55]

https://codereview.chromium.org/971083002/diff/1/mojo/dart/apptest/BUILD.gn
File mojo/dart/apptest/BUILD.gn (right):

https://codereview.chromium.org/971083002/diff/1/mojo/dart/apptest/BUILD.gn#n...
mojo/dart/apptest/BUILD.gn:7: dart_package("apptest") {
On 2015/03/02 23:33:37, Elliot Glaysher wrote:
> On 2015/03/02 22:43:31, tonyg wrote:
> > Are we planning to make this public?
> 
> Yes. I assumed that this dartzip would be a product that could be used in
other
> repos. Is there something further that I have to do around this?

I think Hans and Scott were talking about a solution for this earlier today.

[zra, 2015-03-03 00:45:04]

https://codereview.chromium.org/971083002/diff/20001/services/dart/dart_appte...
File services/dart/dart_apptests/pingpong_apptests.dart (right):

https://codereview.chromium.org/971083002/diff/20001/services/dart/dart_appte...
services/dart/dart_apptests/pingpong_apptests.dart:83: // tests, which is weird.
On 2015/03/02 23:33:37, Elliot Glaysher wrote:
> zra: I'd like some guidance here. What further do I have to do to make make
the
> next line work in the current version of the bindings? Is .impl the wrong
thing
> to pass here?

The first argument to pingTargetService is expected to be an unbound
PingPongServiceStub. The call binds it to one end of a new pipe and sends the
other end to the call at the other end of pingPongServiceProxy.

To make this work you'd need a new implementation of PingPongService that
forwards calls to targetServiceProxy, I guess?

As for why this hangs, the exception thrown from here misses the close calls
below, and so the handles are leaked. I can avoid leaking these handles on an
uncaught exception, but it requires a bit of additional machinery in the
embedder that will take some time to add.

https://codereview.chromium.org/971083002/diff/20001/services/dart/dart_appte...
services/dart/dart_apptests/pingpong_apptests.dart:115:
pingPongServiceProxy.close();
On 2015/03/02 23:40:49, Elliot Glaysher wrote:
> There's also a deeper question about if we can make things work without
explicit
> close() calls. If one of these things throws and unwinds the stack before
> calling close on all these thing, I assume we just hang the program.

This is correct. I'll start working on the necessary finalization in the
unhandled exception callback in the embedder this week.

[Elliot Glaysher, 2015-03-03 22:06:56]

https://codereview.chromium.org/971083002/diff/20001/services/dart/dart_appte...
File services/dart/dart_apptests/pingpong_apptests.dart (right):

https://codereview.chromium.org/971083002/diff/20001/services/dart/dart_appte...
services/dart/dart_apptests/pingpong_apptests.dart:83: // tests, which is weird.
On 2015/03/03 00:45:04, zra wrote:
> On 2015/03/02 23:33:37, Elliot Glaysher wrote:
> > zra: I'd like some guidance here. What further do I have to do to make make
> the
> > next line work in the current version of the bindings? Is .impl the wrong
> thing
> > to pass here?
> 
> The first argument to pingTargetService is expected to be an unbound
> PingPongServiceStub. The call binds it to one end of a new pipe and sends the
> other end to the call at the other end of pingPongServiceProxy.
> 
> To make this work you'd need a new implementation of PingPongService that
> forwards calls to targetServiceProxy, I guess?
> 
> As for why this hangs, the exception thrown from here misses the close calls
> below, and so the handles are leaked. I can avoid leaking these handles on an
> uncaught exception, but it requires a bit of additional machinery in the
> embedder that will take some time to add.

So, why does this work in the C++ version of this file then:

// Same as the previous test except that instead of providing the	
// pingpong_target.dart URL, we provide a connection to its PingPongService.	
TEST_F(DartPingPongTest, PingTargetService) {	
  PingPongServicePtr target;	
  application_impl()->ConnectToService("mojo:dart_pingpong_target", &target);	
  bool returned_value = false;	
  PingTargetCallback callback(&returned_value);	
  pingpong_service_->PingTargetService(target.Pass(), 9, callback);	
  EXPECT_TRUE(pingpong_service_.WaitForIncomingMethodCall());	
  EXPECT_TRUE(returned_value);	
  pingpong_service_->Quit();	
}

In the case of the C++ version, we're able to go connect to a service and then
pass the ptr as an argument to PingTargetService().

[Elliot Glaysher, 2015-03-04 00:02:20]

erg@chromium.org changed reviewers:
+ msw@chromium.org

[Elliot Glaysher, 2015-03-04 00:02:21]

+msw to review the changes to the apptesting framework

[msw, 2015-03-04 00:49:50]

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/apptest_runne...
File mojo/tools/apptest_runner.py (right):

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:67: # TODO(erg): Support android and external and
fixture isolation.
Should fixture isolation (via separate shell invocations) be required in this
CL? It seems like that would dictate whether your resetConnections (etc.) work
is required. You should consult Trung.

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/data/apptests
File mojo/tools/data/apptests (right):

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/data/apptests...
mojo/tools/data/apptests:84: "darttest": True,
Maybe this should be something like "type": "dart", where otherwise we assume
the type is "cpp_gtest" or similar (so we can support additional apptest types
here in the future).

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/mopy/dart_app...
File mojo/tools/mopy/dart_apptest.py (right):

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/mopy/dart_app...
mojo/tools/mopy/dart_apptest.py:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
This file is so similar to gtest.py, I assume we'll want to share code.

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/mopy/dart_app...
mojo/tools/mopy/dart_apptest.py:31: if (output is None or output.find("FAIL:")
!= -1):
This should fail on lack of "PASS:" or w/e. The GTEST runner does that to avoid
cases where the shell or test reports some exceptional failure case that doesn't
print "FAIL:", for example:
[ERROR:application_manager.cc(323)] Library not started because library path
'non_existent_dart_apptests.mojo' does not exist."

[zra, 2015-03-04 03:26:19]

On 2015/03/03 22:06:56, Elliot Glaysher wrote:
> So, why does this work in the C++ version of this file then:
> 
> // Same as the previous test except that instead of providing the	
> // pingpong_target.dart URL, we provide a connection to its PingPongService.	
> TEST_F(DartPingPongTest, PingTargetService) {	
>   PingPongServicePtr target;	
>   application_impl()->ConnectToService("mojo:dart_pingpong_target", &target);	
>   bool returned_value = false;	
>   PingTargetCallback callback(&returned_value);	
>   pingpong_service_->PingTargetService(target.Pass(), 9, callback);	
>   EXPECT_TRUE(pingpong_service_.WaitForIncomingMethodCall());	
>   EXPECT_TRUE(returned_value);	
>   pingpong_service_->Quit();	
> }
> 
> In the case of the C++ version, we're able to go connect to a service and then
> pass the ptr as an argument to PingTargetService().

Hmm. Either the C++ bindings are doing some magic, or I don't understand what
this:

PingTargetService(PingPongService service, uint16 count) => (bool ok);

is supposed to mean, in particular the PingPongService argument, and how to use
it. Maybe we should talk it over in person. I'll be coming by for the meeting
tomorrow morning.

[Elliot Glaysher, 2015-03-04 20:17:02]

msw: I've done all the fixes you asked for minus one.

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/apptest_runne...
File mojo/tools/apptest_runner.py (right):

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:67: # TODO(erg): Support android and external and
fixture isolation.
On 2015/03/04 00:49:50, msw wrote:
> Should fixture isolation (via separate shell invocations) be required in this
> CL? It seems like that would dictate whether your resetConnections (etc.) work
> is required. You should consult Trung.

I am going to punt on this for now.

https://codereview.chromium.org/971083002/diff/60001/services/dart/dart_appte...
File services/dart/dart_apptests/pingpong_apptests.dart (right):

https://codereview.chromium.org/971083002/diff/60001/services/dart/dart_appte...
services/dart/dart_apptests/pingpong_apptests.dart:83: // to another service.
Uncomment this once this works.
Updated this comment into a TODO to uncomment this code when the bindings can
handle it.

[msw, 2015-03-04 21:04:32]

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/apptest_runne...
File mojo/tools/apptest_runner.py (right):

https://codereview.chromium.org/971083002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:67: # TODO(erg): Support android and external and
fixture isolation.
On 2015/03/04 20:17:02, Elliot Glaysher wrote:
> On 2015/03/04 00:49:50, msw wrote:
> > Should fixture isolation (via separate shell invocations) be required in
this
> > CL? It seems like that would dictate whether your resetConnections (etc.)
work
> > is required. You should consult Trung.
> 
> I am going to punt on this for now.

shrug, okay.

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/apptest_runne...
File mojo/tools/apptest_runner.py (right):

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:68: # TODO(erg): Support android and external and
fixture isolation.
nit: s/external/launched services/

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:74: else:
nit: maybe this could be extracted to a gtest.py function, like
run_test_with_fixture_isolation... same args as dart_apptest.run_test, plus
launched_services... that might be reasonable to punt for now.

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/dart_app...
File mojo/tools/mopy/dart_apptest.py (right):

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/dart_app...
mojo/tools/mopy/dart_apptest.py:6: import os
nit: not needed

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/dart_app...
mojo/tools/mopy/dart_apptest.py:7: import re
nit: not needed

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/dart_app...
mojo/tools/mopy/dart_apptest.py:8: import subprocess
nit: not needed

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/dart_app...
mojo/tools/mopy/dart_apptest.py:9: import sys
nit: not needed

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/dart_app...
mojo/tools/mopy/dart_apptest.py:15: from mopy.paths import Paths
nit: not needed

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/dart_app...
mojo/tools/mopy/dart_apptest.py:36: output.find("FAIL:") != -1 or
nit: maybe test_util could just have pass and fail string args, because
otherwise this matches gtest.py's run_test.

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/gtest.py
File mojo/tools/mopy/gtest.py (right):

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/gtest.py...
mojo/tools/mopy/gtest.py:8: import subprocess
nit: no longer needed

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/gtest.py...
mojo/tools/mopy/gtest.py:13: from mopy import android
nit: no longer needed

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/gtest.py...
mojo/tools/mopy/gtest.py:16: from mopy.paths import Paths
nit: no longer needed

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/test_uti...
File mojo/tools/mopy/test_util.py (right):

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/test_uti...
mojo/tools/mopy/test_util.py:7: import re
nit: not needed

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/mopy/test_uti...
mojo/tools/mopy/test_util.py:9: import sys
nit: not needed

[Elliot Glaysher, 2015-03-04 22:20:59]

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/apptest_runne...
File mojo/tools/apptest_runner.py (right):

https://codereview.chromium.org/971083002/diff/60001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:74: else:
On 2015/03/04 21:04:31, msw wrote:
> nit: maybe this could be extracted to a gtest.py function, like
> run_test_with_fixture_isolation... same args as dart_apptest.run_test, plus
> launched_services... that might be reasonable to punt for now.

Done. Moves this (and code only called from this chunk) into gtest.py

[Elliot Glaysher, 2015-03-04 22:27:46]

On 2015/03/04 22:20:59, Elliot Glaysher wrote:
>
https://codereview.chromium.org/971083002/diff/60001/mojo/tools/apptest_runne...
> File mojo/tools/apptest_runner.py (right):
> 
>
https://codereview.chromium.org/971083002/diff/60001/mojo/tools/apptest_runne...
> mojo/tools/apptest_runner.py:74: else:
> On 2015/03/04 21:04:31, msw wrote:
> > nit: maybe this could be extracted to a gtest.py function, like
> > run_test_with_fixture_isolation... same args as dart_apptest.run_test, plus
> > launched_services... that might be reasonable to punt for now.
> 
> Done. Moves this (and code only called from this chunk) into gtest.py

(Also, I tested this locally by deliberately breaking an apptest and a
unittest.)

[msw, 2015-03-04 22:36:45]

I guess the python code lgtm with nits...

https://codereview.chromium.org/971083002/diff/100001/mojo/tools/mopy/gtest.py
File mojo/tools/mopy/gtest.py (right):

https://codereview.chromium.org/971083002/diff/100001/mojo/tools/mopy/gtest.p...
mojo/tools/mopy/gtest.py:26: def run_fixture(config, apptest_dict, apptest,
test_args, shell_args,
"fixture" is the term that GTest uses to describe a single TEST(), ie. a member
of a test [suite]. Rename this "run_fixtures", "run_tests", or similar.

https://codereview.chromium.org/971083002/diff/100001/mojo/tools/mopy/gtest.p...
mojo/tools/mopy/gtest.py:28: """Sets up the fixture to run gtests."""
Ditto, re-write this as "Run the gtest fixtures in isolation." or similar.

https://codereview.chromium.org/971083002/diff/100001/mojo/tools/mopy/gtest.p...
mojo/tools/mopy/gtest.py:32: # List the apptest fixtures so they can be run
independently for
nit: fix wrapping, "isolation." fits on this line again.

https://codereview.chromium.org/971083002/diff/100001/mojo/tools/mopy/gtest.p...
mojo/tools/mopy/gtest.py:46: success = RunApptestInLauncher(config, mojo_paths,
apptest,
nit: fix wrapping, "apptest_args," fits on this line again.

[Elliot Glaysher, 2015-03-04 23:04:35]

ping tonyg or zra to review the dart code.

https://codereview.chromium.org/971083002/diff/1/third_party/dart-packages/ma...
File third_party/dart-packages/matcher/BUILD.gn (right):

https://codereview.chromium.org/971083002/diff/1/third_party/dart-packages/ma...
third_party/dart-packages/matcher/BUILD.gn:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/03/02 22:43:31, tonyg wrote:
> These BUILD.gn files should be in mirror directories in build/secondary,
right?
> Then eventually these directories could be pulled in via DEPS.
> 
> Also, these packages also exist in sky/tests/resources/third_party. When this
> lands, we should make sure that they're only in one place in the repo.

The version there has been modified; all import statements have been rewritten
to be relative paths.

[tonyg, 2015-03-04 23:13:55]

lgtm

https://codereview.chromium.org/971083002/diff/120001/mojo/dart/apptest/appte...
File mojo/dart/apptest/apptest/apptest.dart (right):

https://codereview.chromium.org/971083002/diff/120001/mojo/dart/apptest/appte...
mojo/dart/apptest/apptest/apptest.dart:20: List<Function> testFunctions;
final?

https://codereview.chromium.org/971083002/diff/120001/mojo/dart/apptest/appte...
mojo/dart/apptest/apptest/apptest.dart:30: for (var fun in testFunctions) {
testFunctions.forEach((f) => f(this));

https://codereview.chromium.org/971083002/diff/120001/mojo/dart/apptest/appte...
mojo/dart/apptest/apptest/apptest.dart:38: _ConnectionToShellApplication
_application;
final?

https://codereview.chromium.org/971083002/diff/120001/mojo/dart/apptest/appte...
mojo/dart/apptest/apptest/apptest.dart:58: runAppTests(var incoming_handle,
List<Function> testFunction) {
incomingHandle

https://codereview.chromium.org/971083002/diff/120001/mojo/dart/apptest/appte...
mojo/dart/apptest/apptest/apptest.dart:59: MojoHandle appHandle = new
MojoHandle(incoming_handle);
var?

https://codereview.chromium.org/971083002/diff/120001/mojo/dart/apptest/appte...
mojo/dart/apptest/apptest/apptest.dart:62: unittestConfiguration = new
_CleanShutdownConfiguration(application);
var or just remove the lhs?

[Elliot Glaysher, 2015-03-04 23:51:16]

tonyg: next review. I've updated the upload_binaries.py file to add the dartzip
thing.

When performing a dry run, I get:
['/work/chrome/depot_tools/third_party/gsutil/gsutil', 'cp',
'/work/mojo/src/out/Release/obj/mojo/dart/apptest/apptest.dartzip',
'gs://mojo/services/linux-x64/f6ea5b4e4de67d7535028146e423bf06522451df/apptest.dartzip']

I'll ask you the next question in person.

https://codereview.chromium.org/971083002/diff/120001/mojo/dart/apptest/appte...
File mojo/dart/apptest/apptest/apptest.dart (right):

https://codereview.chromium.org/971083002/diff/120001/mojo/dart/apptest/appte...
mojo/dart/apptest/apptest/apptest.dart:62: unittestConfiguration = new
_CleanShutdownConfiguration(application);
On 2015/03/04 23:13:54, tonyg wrote:
> var or just remove the lhs?

unittestConfiguration is a global variable in the unittest package that I am
setting.

[tonyg, 2015-03-05 00:03:24]

lgtm

https://codereview.chromium.org/971083002/diff/140001/mojo/tools/upload_binar...
File mojo/tools/upload_binaries.py (right):

https://codereview.chromium.org/971083002/diff/140001/mojo/tools/upload_binar...
mojo/tools/upload_binaries.py:131: gsutil_file_location = "gs://" +
file_location_in_gs
Would be nice to factor out a helper for building these paths that can be shared
with upload_app(). I'm guessing if we changed one, we'd want to notice and
change the other.

[Elliot Glaysher, 2015-03-05 00:08:40]

Committed patchset #8 (id:140001) manually as
373b74f1339c7953007662dd5237d8507e156655 (presubmit successful).