Adding Test Fixture for initial test cases for the App Remoting Test Driver. Also includes the pub…

remoting/test/app_remoting_connected_client_tests.cc

Index: remoting/test/app_remoting_connected_client_tests.cc
diff --git a/remoting/test/app_remoting_connected_client_tests.cc b/remoting/test/app_remoting_connected_client_tests.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cd10085fa342a2937f2440341f01b323f5a4b70a
--- /dev/null
+++ b/remoting/test/app_remoting_connected_client_tests.cc
@@ -0,0 +1,256 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/test/app_remoting_connected_client_tests.h"
+
+#include "base/json/json_reader.h"
+#include "base/logging.h"
+#include "base/run_loop.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/values.h"
+#include "remoting/protocol/host_stub.h"
+#include "remoting/test/app_remoting_test_driver_environment.h"
+#include "remoting/test/remote_application_data.h"
+#include "remoting/test/test_chromoting_client.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+const int kDefaultDPI = 96;
+const int kDefaultWidth = 1024;
+const int kDefaultHeight = 768;
+
+void SimpleHostMessageReceivedHandler(
+    const std::string& target_message_type,
+    const std::string& target_message_data,
+    const base::Closure& done_closure,
+    bool* message_received,
+    const remoting::protocol::ExtensionMessage& message) {
+  if (message.type() == target_message_type &&
+      message.data() == target_message_data) {
+    *message_received = true;
+    done_closure.Run();
+  }
+}
+}

[Wez, 2015/03/16 22:19:08]

nit: This is a non-trivial namespace declaration, so put a // namespace comment
at the end.

[joedow, 2015/03/18 20:13:08]

Done.

+
+namespace remoting {
+namespace test {
+
+scoped_ptr<base::MessageLoopForIO>
+    AppRemotingConnectedClientTests::message_loop_;

[Wez, 2015/03/16 22:19:08]

Does this line really need wrapping?

[joedow, 2015/03/18 20:13:08]

It does :(

+scoped_ptr<TestChromotingClient> AppRemotingConnectedClientTests::client_;
+bool AppRemotingConnectedClientTests::connection_is_ready_for_tests_;
+
+AppRemotingConnectedClientTests::AppRemotingConnectedClientTests()
+    : application_info_(GetRemoteApplicationInfoMap().at(GetParam())) {
+}
+
+AppRemotingConnectedClientTests::~AppRemotingConnectedClientTests() {
+}
+
+void AppRemotingConnectedClientTests::SetUp() {
+  // NOTE: Code here will be called immediately after the constructor (right
+  // before each test).

[Wez, 2015/03/16 22:19:09]

Which of these properties is the important one to note?

[joedow, 2015/03/18 20:13:08]

Done.  Removed since I got rid of the static methods so there shouldn't be any
confusion as to when these run now.

+  DCHECK(client_);

[Wez, 2015/03/16 22:19:09]

No point DCHECKing a pointer immediately before dereferencing it!

In DEBUG builds scoped_ptr<> will actually DCHECK for you, in fact.

[joedow, 2015/03/18 20:13:08]

Done.

+  client_->AddRemoteConnectionObserver(this);
+
+  // If the client has not attempted to create a connection in the past, then
+  // start one now.
+  if (client_->connection_to_host_state() ==
+          protocol::ConnectionToHost::INITIALIZING &&
+      client_->connection_error_code() == protocol::OK) {
+    StartConnection();
+  }
+
+  if (!connection_is_ready_for_tests_) {
+    FAIL() << "Remote host connection could not be completed.";
+    client_->EndConnection();
+  }
+}
+
+void AppRemotingConnectedClientTests::TearDown() {
+  // NOTE: Code here will be called immediately after each test (right
+  // before the destructor).
+  DCHECK(client_);

[Wez, 2015/03/16 22:19:09]

See above re DCHECK

[joedow, 2015/03/18 20:13:08]

Done.

+  client_->RemoveRemoteConnectionObserver(this);
+}
+
+void AppRemotingConnectedClientTests::SetUpTestCase() {
+  // NOTE: Code here will be called before any instance constructors are called.
+  DCHECK(!message_loop_);
+  message_loop_.reset(new base::MessageLoopForIO);
+
+  DCHECK(!client_);
+  client_.reset(new TestChromotingClient());
+}
+
+void AppRemotingConnectedClientTests::TearDownTestCase() {
+  // NOTE: Code here will be called immediately after all tests are done.
+  connection_is_ready_for_tests_ = false;
+
+  // The client must be destroyed before the message loop as some of its
+  // members are destroyed via DeleteSoon on the message loop's TaskRunner.

[Wez, 2015/03/16 22:19:09]

Unless you actually run the message-loop after destroying the client, though, it
won't execute any tasks that are posted to it.

If you want that to happen then I'd suggest adding:

base::RunLoop().RunUntilIdle();

between the |client_| and |message_loop_| reset() calls.

[joedow, 2015/03/18 20:13:08]

I had thought this was happening in the D'Tor for |client_| but I was wrong,
added it here.

+  client_.reset();
+  message_loop_.reset();
+}
+
+bool AppRemotingConnectedClientTests::VerifyResponseForSimpleHostMessage(
+    const std::string& message_request_title,
+    const std::string& message_response_title,
+    const std::string& message_payload,
+    const base::TimeDelta& max_wait_time) {
+  base::RunLoop run_loop;
+  bool message_received = false;
+
+  done_closure_ = run_loop.QuitClosure();

[Wez, 2015/03/16 22:19:08]

Why are you saving the QuitClosure() for a RunLoop you've allocated on the stack
into an object member variable?

[joedow, 2015/03/18 20:13:09]

Done.

+  host_message_received_callback_ =
+      base::Bind(&SimpleHostMessageReceivedHandler, message_response_title,
+                 message_payload, done_closure_, &message_received);
+
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(FROM_HERE, done_closure_,
+                                                       max_wait_time);

[Wez, 2015/03/16 22:19:09]

Cleaner to use a base::Timer for this?

[joedow, 2015/03/18 20:13:08]

Done.

+
+  protocol::ExtensionMessage message;
+  message.set_type(message_request_title);
+  message.set_data(message_payload);
+  client_->host_stub()->DeliverClientMessage(message);
+
+  run_loop.Run();
+  host_message_received_callback_.Reset();
+
+  return message_received;
+}
+
+void AppRemotingConnectedClientTests::StartConnection() {
+  DCHECK(client_);
+
+  RemoteHostInfo remote_host_info;
+  remoting::test::AppRemotingSharedData->GetRemoteHostInfoForApplicationId(
+      application_info_.application_id, &remote_host_info);
+
+  if (!remote_host_info.IsReadyForConnection()) {
+    LOG(ERROR) << "Remote Host is unavailable for connections.";
+    return;
+  }
+
+  base::RunLoop run_loop;
+
+  done_closure_ = run_loop.QuitClosure();

[Wez, 2015/03/16 22:19:09]

See above, re why this needs saving.

[joedow, 2015/03/18 20:13:08]

Done.

+  host_message_received_callback_ =
+      base::Bind(&AppRemotingConnectedClientTests::AddWindowMessageHandler,
+                 base::Unretained(this), application_info_.main_window_name);
+
+  // We will wait up to 30 seconds to complete the remote connection and for the
+  // main application window to become visible.
+  base::TimeDelta wait_time = base::TimeDelta::FromSeconds(30);
+
+  // Post the QuitClosure with a max timeout to ensure we don't run forever.
+  // The QuitClosure will NOP if the connection was created before the timeout.
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(FROM_HERE, done_closure_,
+                                                       wait_time);

[Wez, 2015/03/16 22:19:09]

See above re base::Timer

[joedow, 2015/03/18 20:13:08]

Done.

+
+  client_->StartConnection(AppRemotingSharedData->user_name(),
+                           AppRemotingSharedData->access_token(),
+                           remote_host_info);
+
+  run_loop.Run();
+  host_message_received_callback_.Reset();
+}
+
+void AppRemotingConnectedClientTests::ConnectionStateChanged(

[Wez, 2015/03/16 22:19:08]

The RemoteConnectionObserver APIs are called by the |client_| - what thread will
that use? Consider DCHECKing that the thread is the same as the one on which the
test-case was instantiated.

[joedow, 2015/03/18 20:13:08]

Done.

+    protocol::ConnectionToHost::State state,
+    protocol::ErrorCode error_code) {
+  if (state != protocol::ConnectionToHost::CONNECTED ||
+      error_code != protocol::OK) {
+    connection_is_ready_for_tests_ = false;
+  }
+
+  if (!done_closure_.is_null() &&
+      (state == protocol::ConnectionToHost::CLOSED ||
+       state == protocol::ConnectionToHost::FAILED ||
+       error_code != protocol::OK)) {

[Wez, 2015/03/16 22:19:08]

Why are these two connection-state checks different? e.g. When would we ever
want to transition into the ready-for-tests=false state without quitting the
test run-loop?

[joedow, 2015/03/18 20:13:08]

Done.

+    done_closure_.Run();
+  }
+}
+
+void AppRemotingConnectedClientTests::ConnectionReady(bool ready) {

[Wez, 2015/03/16 22:19:09]

Under what circumstances will we receive this call with |ready| false? Should we
be running |done_callback_| in those cases?

[joedow, 2015/03/18 20:13:08]

Done.

+  if (ready) {
+    SendClientConnectionDetailsToHost();
+  }
+}
+
+void AppRemotingConnectedClientTests::HostMessageReceived(
+    const protocol::ExtensionMessage& message) {
+  if (!host_message_received_callback_.is_null()) {

[Wez, 2015/03/16 22:19:09]

Do we really want to ignore host messages received while we're not expecting
them?

[joedow, 2015/03/18 20:13:08]

I've updated this method so that we can add default message handlers to the
fixture (along with some logging to indicate when messages are received which
aren't handled) such as the OnWindowAddedHandler.

+    host_message_received_callback_.Run(message);
+  }
+}
+
+void AppRemotingConnectedClientTests::SendClientConnectionDetailsToHost() {
+  // First send an access token which will be used for GDrive access.
+  protocol::ExtensionMessage message;
+  message.set_type("accessToken");
+  message.set_data(AppRemotingSharedData->access_token());
+
+  DVLOG(1) << "Sending access token to host";
+  client_->host_stub()->DeliverClientMessage(message);
+
+  // next send the host a description of the client screen size.

[Wez, 2015/03/16 22:19:09]

nit: Capitalization

[joedow, 2015/03/18 20:13:08]

Done.

+  protocol::ClientResolution client_resolution;
+  client_resolution.set_width(kDefaultWidth);
+  client_resolution.set_height(kDefaultHeight);
+  client_resolution.set_x_dpi(kDefaultDPI);
+  client_resolution.set_y_dpi(kDefaultDPI);
+  client_resolution.set_dips_width(kDefaultWidth);
+  client_resolution.set_dips_height(kDefaultHeight);
+
+  DVLOG(1) << "Sending ClientResolution details to host";
+  client_->host_stub()->NotifyClientResolution(client_resolution);
+
+  // Finally send a message to start sending us video packets.
+  protocol::VideoControl video_control;
+  video_control.set_enable(true);
+

[Wez, 2015/03/16 22:19:08]

nit: No need for this blank line.

[joedow, 2015/03/18 20:13:08]

Done.

+  video_control.set_lossless_encode(true);
+  video_control.set_lossless_color(true);

[Wez, 2015/03/16 22:19:08]

By default these are currently off (false).

[joedow, 2015/03/18 20:13:09]

Done.

+
+  DVLOG(1) << "Sending enable VideoControl message to host";
+  client_->host_stub()->ControlVideo(video_control);
+}
+
+void AppRemotingConnectedClientTests::AddWindowMessageHandler(
+    const std::string& main_window_name,

[Wez, 2015/03/16 22:19:08]

It seems strange to pass main_window_name in as a bound parameter, even though
this handler function is a member of the class, so has access to members of the
test instance object itself.

[joedow, 2015/03/18 20:13:08]

This is an artifact of some refactoring I did, you are totally right that it
doesn't make sense.

+    const remoting::protocol::ExtensionMessage& message) {
+  if (message.type() == "onWindowAdded") {
+    const base::DictionaryValue* message_data = nullptr;
+    scoped_ptr<base::Value> host_message(
+        base::JSONReader::Read(message.data()));
+    if (!host_message.get() || !host_message->GetAsDictionary(&message_data)) {
+      LOG(ERROR) << "Message received was not valid JSON.";

[Wez, 2015/03/16 22:19:08]

Should we terminate the test (e.g. via |done_callback_| in this case?

[joedow, 2015/03/18 20:13:08]

Done.

+      return;
+    }
+
+    std::string current_window_title;
+    message_data->GetString("title", &current_window_title);
+
+    if (current_window_title == kHostProcessWindowTitle) {
+      LOG(WARNING) << "Host Process Window is visible, this may mean that the "
+                   << "underlying application is in a bad state, YMMV.";

[Wez, 2015/03/16 22:19:08]

I'm surprised that this is ever a state we get into! Is this something you see
in real test runs?

[joedow, 2015/03/18 20:13:08]

I saw this last year when we had created a snapshot where the host kept
crashing.  In that scenario the app would start up but the Host Process window
would pop and show up in front of the application window.  I'd like to log this
as it is a bad thing, I'm thinking it should be logged as an error though.  I'll
update the severity.

+    }
+
+    std::string main_window_title = main_window_name;
+    if (current_window_title.find_first_of(main_window_title) == 0) {
+      connection_is_ready_for_tests_ = true;
+
+      // Now that the main window is visible, give the app some time to settle
+      // before signaling that it is ready to run tests.
+      base::TimeDelta wait_time = base::TimeDelta::FromSeconds(5);
+      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE, done_closure_, wait_time);

[Wez, 2015/03/16 22:19:08]

Consider replacing this with a base::Timer member of the test object.

[joedow, 2015/03/18 20:13:08]

Done.

+    }
+  }

[Wez, 2015/03/16 22:19:08]

Do we want to completely ignore all other window messages? Or at least verify
that they are one of the ones we expect / know about, and fail otherwise?

[joedow, 2015/03/18 20:13:08]

I'd like to keep this method simple for now, we can add more logic later if we
want to handle sequences of messages.  Once this tool is in place, I am going to
gather scenarios and figure out what else we would need.  

+}
+
+}  // namespace test
+}  // namespace remoting