syncbase: Echo and syncbase tests run, pass, and exit reliably.
The biggest issue was that when --enable-multiprocess is set (as it must
be for Go/Mojo apps), closing the main application causes mojo_shell to
crash. Not closing an application, however, causes mojo_shell to hang.
The solution is to wait for the test reporter to finish, then close the
application. mojo_shell still crashes, but the test harness is able to
find the test reporter output and exit with the correct status code.
Calling a "Quit" method in Syncbase made no difference. I got rid of
the "Quit" method in the echo_server, since we weren't using it anyways.
This brought a tear to my eye:
Running dart/test/echo_test.dart.... Succeeded
Running dart/test/syncbase_test.dart.... Succeeded
Change-Id: Idd25d5369cd5dc19c572d2e7150b0e26253ab75d
diff --git a/Makefile b/Makefile
index b8f4289..b4625e0 100644
--- a/Makefile
+++ b/Makefile
@@ -8,14 +8,28 @@
# Flags for Syncbase service running as Mojo service.
# See v.io/x/ref/runtime/internal/mojo_util.go for the wonderful magic that
# makes this work.
-MOUNTTABLE_ADDR := 127.0.0.1:4001
+# TODO(nlacasse): The --name flag was causing the tests to take a long time to
+# exit because the server was trying to resolve a mounttable that doesn't
+# exist. I ripped the --name flag out. Adam is fixing this the right way in a
+# forthcoming CL.
+MOUNTABLE_ADDR := 127.0.0.1:4001
SYNCBASED_ADDR := 127.0.0.1:4002
-V23_MOJO_FLAGS := '--v=5 --alsologtostderr=false --root-dir=/tmp/syncbase_mojo --name=syncbase_mojo --v23.namespace.root=/$(MOUNTTABLE_ADDR) --v23.tcp.address=$(SYNCBASED_ADDR) --v23.permissions.literal={"Admin":{"In":["..."]},"Write":{"In":["..."]},"Read":{"In":["..."]},"Resolve":{"In":["..."]},"Debug":{"In":["..."]}} --v23.credentials=$(V23_ROOT)/experimental/projects/ether/creds'
+V23_MOJO_FLAGS := '--v=5 --alsologtostderr=false --root-dir=/tmp/syncbase_mojo --v23.tcp.address=$(SYNCBASED_ADDR) --v23.permissions.literal={"Admin":{"In":["..."]},"Write":{"In":["..."]},"Read":{"In":["..."]},"Resolve":{"In":["..."]},"Debug":{"In":["..."]}} --v23.credentials=$(V23_ROOT)/experimental/projects/ether/creds'
ifndef MOJO_DIR
- $(error MOJO_DIR is not set: $(MOJO_DIR))
+ $(error MOJO_DIR is not set)
endif
+ifndef V23_ROOT
+ $(error V23_ROOT is not set)
+endif
+
+# NOTE(nlacasse): Running Go Mojo services requires passing the
+# --enable-multiprocess flag to mojo_shell. This is because the Go runtime is
+# very large, and can interfere with C++ memory if they are in the same
+# process.
+MOJO_SHELL_FLAGS := -v --enable-multiprocess
+
ifdef ANDROID
# TODO(nlacasse): Serve mojo resources over HTTP so they are accessible to
# Android. Currently everything is served off the local filesystem, which
@@ -26,8 +40,8 @@
GO_BIN := $(MOJO_DIR)/src/third_party/go/tool/android_arm/bin/go
GO_FLAGS := -tags=mojo -ldflags=-shared
+ MOJO_ANDROID_FLAGS := --android
MOJO_BUILD_DIR := $(MOJO_DIR)/src/out/android_Debug
- MOJO_FLAGS := --android
MOJO_SHARED_LIB := $(PWD)/gen/lib/android/libsystem_thunk.a
SYNCBASE_LEVELDB_DIR := $(V23_ROOT)/third_party/cout/linux_amd64/leveldb
@@ -37,7 +51,6 @@
GO_BIN := $(MOJO_DIR)/src/third_party/go/tool/linux_amd64/bin/go
GO_FLAGS := -tags=mojo -ldflags=-shared -buildmode=c-shared
- MOJO_FLAGS :=
MOJO_BUILD_DIR := $(MOJO_DIR)/src/out/Debug
MOJO_SHARED_LIB := $(PWD)/gen/lib/linux_amd64/libsystem_thunk.a
@@ -62,7 +75,7 @@
MOGO_BIN := $(MOJO_DIR)/src/mojo/go/go.py
define MOGO_BUILD
mkdir -p $(dir $2)
- $(MOGO_BIN) $(MOJO_FLAGS) -- \
+ $(MOGO_BIN) $(MOJO_ANDROID_FLAGS) -- \
"$(GO_BIN)" \
"$(shell mktemp -d)" \
"$(PWD)/$(2)" \
@@ -161,18 +174,11 @@
# are stable enough to reliably test syncbase.
.PHONY: run-syncbase-app
run-syncbase-app: gen/mojo/syncbase_server.mojo dart/packages dart/lib/gen/dart-pkg/mojom/lib/mojo/syncbase.mojom.dart
- V23_MOJO_FLAGS=$(V23_MOJO_FLAGS) $(MOJO_DIR)/src/mojo/devtools/common/mojo_run $(MOJO_FLAGS) -v --enable-multiprocess $(PWD)/dart/bin/syncbase.dart
+ V23_MOJO_FLAGS=$(V23_MOJO_FLAGS) $(MOJO_DIR)/src/mojo/devtools/common/mojo_run $(MOJO_SHELL_FLAGS) $(MOJO_ANDROID_FLAGS) $(PWD)/dart/bin/syncbase.dart
.PHONY: test
-test: dart/packages gen/mojo/echo_server.mojo gen-mojom
- # TODO(nlacasse): These tests sometimes hang. I suspect some connection is
- # not getting closed properly. More debugging is necessary.
- # TODO(nlacasse): We should be passing "--enable-multiprocess" here, since
- # that is usually needed for Go Mojo services. However, using that flag
- # causes the test runner to crash on exit with "Connection error to the
- # shell". The tests somehow run and pass without that flag, so maybe it's
- # not necessary?
- $(MOJO_DIR)/src/mojo/devtools/common/mojo_test $(MOJO_FLAGS) -v --shell-path $(MOJO_DIR)/src/out/Debug/mojo_shell tests
+test: dart/packages gen/mojo/echo_server.mojo gen/mojo/syncbase_server.mojo gen-mojom
+ V23_MOJO_FLAGS=$(V23_MOJO_FLAGS) $(MOJO_DIR)/src/mojo/devtools/common/mojo_test $(MOJO_SHELL_FLAGS) $(MOJO_ANDROID_FLAGS) --shell-path $(MOJO_DIR)/src/out/Debug/mojo_shell tests
.PHONY: clean
clean:
diff --git a/README.md b/README.md
index 6cf1514..53855f3 100644
--- a/README.md
+++ b/README.md
@@ -82,7 +82,7 @@
The following command will run a single test file. This is useful when the
full test suite hangs with no output.
- $(MOJO_DIR)/src/mojo/devtools/common/mojo_run $(MOJO_FLAGS) -v --shell-path $(MOJO_DIR)/src/out/Debug/mojo_shell dart/test/<filename>
+ $(MOJO_DIR)/src/mojo/devtools/common/mojo_run -v --enable-multiprocess --shell-path $(MOJO_DIR)/src/out/Debug/mojo_shell dart/test/<filename>
[architecture proposal]: https://docs.google.com/document/d/1TyxPYIhj9VBCtY7eAXu_MEV9y0dtRx7n7UY4jm76Qq4/edit
[depot tools]: http://www.chromium.org/developers/how-tos/install-depot-tools
diff --git a/dart/lib/echo_client.dart b/dart/lib/echo_client.dart
index 593518a..3954cea 100644
--- a/dart/lib/echo_client.dart
+++ b/dart/lib/echo_client.dart
@@ -11,6 +11,10 @@
final mojom.EchoProxy _proxy;
final String url;
+ Future close({bool immediate: false}) {
+ return _proxy.close();
+ }
+
EchoClient(this._app, this.url) : _proxy = new mojom.EchoProxy.unbound() {
print('connecting to $url');
_app.connectToService(url, _proxy);
diff --git a/dart/lib/syncbase_client.dart b/dart/lib/syncbase_client.dart
index 4f895db..4e0d897 100644
--- a/dart/lib/syncbase_client.dart
+++ b/dart/lib/syncbase_client.dart
@@ -26,6 +26,10 @@
print('connected');
}
+ Future close({bool immediate: false}) {
+ return _proxy.close();
+ }
+
// TODO(nlacasse): Break this SyncbaseClient class into multiple classes, one
// for each level.
diff --git a/dart/test/echo_test.dart b/dart/test/echo_test.dart
index 7b30525..df9fe34 100644
--- a/dart/test/echo_test.dart
+++ b/dart/test/echo_test.dart
@@ -37,7 +37,15 @@
// TODO(nlacasse): Remove this once package 'test' supports a global tearDown
// callback. See https://github.com/dart-lang/test/issues/18.
test('terminate shell connection', () async {
- await app.close();
+ await c.close();
expect(MojoHandle.reportLeakedHandles(), isTrue);
+
+ // TODO(nlacasse): When running mojo_shell with --enable-multiprocess,
+ // closing the application causes a non-graceful shutdown. To avoid this,
+ // we sleep for a second so the test reporter can finish and print the
+ // results before we close the app. Once mojo_shell can shut down more
+ // gracefully, we should call app.close directly in the test and not in
+ // this Timer.
+ new Timer(new Duration(seconds: 1), app.close);
});
}
diff --git a/dart/test/syncbase_test.dart b/dart/test/syncbase_test.dart
index 558dead..d8bcb3f 100755
--- a/dart/test/syncbase_test.dart
+++ b/dart/test/syncbase_test.dart
@@ -1,4 +1,7 @@
#!mojo mojo:dart_content_handler
+
+import 'dart:async';
+
import 'package:mojo/core.dart' show MojoHandle;
import 'package:test/test.dart';
@@ -20,14 +23,22 @@
});
test('appExists(foo) should be false', () {
- expect(c.appExists, completion(isFalse));
+ expect(c.appExists('foo'), completion(isFalse));
});
// Append a final test to terminate shell connection.
// TODO(nlacasse): Remove this once package 'test' supports a global tearDown
// callback. See https://github.com/dart-lang/test/issues/18.
test('terminate shell connection', () async {
- await app.close();
+ await c.close();
expect(MojoHandle.reportLeakedHandles(), isTrue);
+
+ // TODO(nlacasse): When running mojo_shell with --enable-multiprocess,
+ // closing the application causes a non-graceful shutdown. To avoid this,
+ // we sleep for a second so the test reporter can finish and print the
+ // results before we close the app. Once mojo_shell can shut down more
+ // gracefully, we should call app.close directly in the test and not in
+ // this Timer.
+ new Timer(new Duration(seconds: 1), app.close);
});
}
diff --git a/go/src/echo_server.go b/go/src/echo_server.go
index b0c1a14..29323c5 100644
--- a/go/src/echo_server.go
+++ b/go/src/echo_server.go
@@ -23,7 +23,6 @@
import "C"
type echoImpl struct {
- stub *bindings.Stub
}
func (e *echoImpl) EchoString(in *string) (out *string, err error) {
@@ -31,11 +30,6 @@
return in, nil
}
-func (echo *echoImpl) Quit() error {
- echo.stub.Close()
- return nil
-}
-
type delegate struct {
stubs []*bindings.Stub
}
@@ -43,9 +37,7 @@
func (d *delegate) Initialize(ctx application.Context) {}
func (d *delegate) Create(req echo.Echo_Request) {
- impl := &echoImpl{}
- stub := echo.NewEchoStub(req, impl, bindings.GetAsyncWaiter())
- impl.stub = stub
+ stub := echo.NewEchoStub(req, &echoImpl{}, bindings.GetAsyncWaiter())
d.stubs = append(d.stubs, stub)
go func() {
for {
diff --git a/mojom/echo.mojom b/mojom/echo.mojom
index 62d7ccf..14ca169 100644
--- a/mojom/echo.mojom
+++ b/mojom/echo.mojom
@@ -6,21 +6,4 @@
interface Echo {
EchoString(string? value) => (string? value);
-
- // TODO(nlacasse): The test runner currently needs this Quit method to tell
- // the echo service to quit, otherwise the tests hang because the echo
- // service never stops running and listening for connections.
- //
- // The Mojo dart tests that this is based on do essentially the same thing.
- // See $MOJO_DIR/src/services/dart/dart_apptests/echo_apptests.dart
- //
- // The "Quitting Mojo Apps" document
- // (https://drive.google.com/a/google.com/folderview?id=0B-WCZkfLIXQTajI4WWZhdGhjNnM)
- // says that Mojo applications should gracefully quit when Mojo shell
- // terminates (see Condition C in that doc), but that does not appear to be
- // implemented yet.
- //
- // Once the Mojo app can gracefully terminate on shell exit, we should get
- // rid of this method.
- Quit();
};
diff --git a/tests b/tests
index 5bacde1..277f3ce 100644
--- a/tests
+++ b/tests
@@ -4,14 +4,11 @@
{
"test": "dart/test/echo_test.dart",
"type": "dart",
- "timeout":5
+ "timeout": 10
},
- # TODO(nlacasse): The syncbase tests run and pass, but then mojo_shell does
- # not exit. I suspect some connection is not getting closed properly.
- # More debugging is necessary.
- #{
- # "test": "dart/test/syncbase_test.dart",
- # "type": "dart",
- # "timeout": 5
- #}
+ {
+ "test": "dart/test/syncbase_test.dart",
+ "type": "dart",
+ "timeout": 10
+ }
]
