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

remoting/test/app_remoting_connected_client_tests.h

+// 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.
+
+#ifndef REMOTING_TEST_APP_REMOTING_CONNECTED_CLIENT_TESTS_H_
+#define REMOTING_TEST_APP_REMOTING_CONNECTED_CLIENT_TESTS_H_
+
+#include <string>
+
+#include "base/callback_forward.h"
+#include "base/memory/scoped_ptr.h"
+#include "remoting/test/remote_connection_observer.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+class MessageLoopForIO;
+}
+
+namespace remoting {
+namespace test {
+
+struct ApplicationInfo;
+class TestChromotingClient;
+
+// Supplied by the test code to the fixture to allow for custom handling of
+// specific messages.
+typedef base::Callback<void(const protocol::ExtensionMessage& message)>
+    HostMessageReceivedCallback;
+
+// Creates a connection to a remote host when the static SetUpTestCase() method

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

This comment should describe the overall intended purpose of this fixture (i.e.
to set up common test state such as the connection to the host) and then
describe the things it does to achieve that, if you need to; as it is it is very
implementation-centric!

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

Done.

+// is called.  This connection is persisted until TearDownTestCase() is called.
+// This means that tests which use this fixture can rely on having a connection

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

"This means that" is superfluous.

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

Done.

+// available to them and do not need to create/manage it themselves.
+// The parameter that is used to instantiate a test using this fixture is the
+// name of the application we want to test.

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

This sentence is pretty confusingly worded!

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

My mastery of the English language changes depending on how much sleep my little
one allows me to have :)  I'll rewrite this and see if it is more coherent.

+class AppRemotingConnectedClientTests

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

Why is this class "*Tests" rather than "*Test"? Each derived instance is a
single test, isn't it, not multiple tests?

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

I've seen 'Tests' used for fixtures but it looks like Fixture is more popular in
chromium, I'll update the name.

+    : public testing::TestWithParam<const char*>,
+      public RemoteConnectionObserver {
+ public:
+  AppRemotingConnectedClientTests();
+  ~AppRemotingConnectedClientTests() override;
+
+  // static testing::Test overrides.

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

Since these are static, they are presumably overrides only for this
instantiation of the  testing::TestWithParam template?

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

Acknowledged.

+  static void SetUpTestCase();
+  static void TearDownTestCase();
+
+ protected:
+  bool connection_is_ready_for_tests() {
+    return connection_is_ready_for_tests_;
+  }
+
+  // Sends the request to the host and waits for a reply up to |max_wait_time|.
+  // Returns true if we received a response within the maximum time limit.
+  bool VerifyResponseForSimpleHostMessage(
+      const std::string& message_request_title,
+      const std::string& message_response_title,
+      const std::string& message_payload,
+      const base::TimeDelta& max_wait_time);
+
+ private:
+  // testing::Test interface.
+  void SetUp() override;
+  void TearDown() override;
+
+  // RemoteConnectionObserver interface.
+  void ConnectionStateChanged(protocol::ConnectionToHost::State state,
+                              protocol::ErrorCode error_code) override;
+  void ConnectionReady(bool ready) override;
+  void HostMessageReceived(const protocol::ExtensionMessage& message) override;
+
+  // Starts a connection with the remote host defined by the parameter..

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

typo: spurious .

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

Done.

+  void StartConnection();
+
+  // Sends the final client details to the host in order to complete the
+  // connection.
+  void SendClientConnectionDetailsToHost();
+
+  // Runs |done_closure_| when an onWindowAdded message with |main_window_name|
+  // for the title is passed in.

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

This comment is confusing since it doesn't relate anything back to |message|
directly, and its name doesn't seem to fit its function, going by the comment -
it seems that this method actually calls |done_closure_| if the contents of
|message| match |main_window_name|, in which case shouldn't this be
CallDoneCallbackIfWindowMatches() or similar?

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

Done.

+  void AddWindowMessageHandler(
+      const std::string& main_window_name,
+      const remoting::protocol::ExtensionMessage& message);
+
+  // Tracks the details for the application the tests are targeting.
+  const ApplicationInfo& application_info_;
+
+  // Used to signal when an action has been completed.
+  base::Closure done_closure_;
+
+  // Used to customize the message handling behavior between tests.  Called when
+  // a host message is received.
+  HostMessageReceivedCallback host_message_received_callback_;

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

It's not clear how tests can customize this, since it's a private member and
there are no protected nor public APIs to set it?

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

Done.

+
+  // Tracks the state of the connection between tests.  True when we have a
+  // chromoting connection to the remote host established and the main
+  // application window is visible.
+  static bool connection_is_ready_for_tests_;
+
+  // Used to post tasks by |client_| and tests.
+  static scoped_ptr<base::MessageLoopForIO> message_loop_;
+
+  // Creates and manages the connection to the remote host.
+  static scoped_ptr<TestChromotingClient> client_;

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

Do we really want to re-use a connection across tests?

That means tests may acquire implicit dependencies upon one another and break if
they are re-ordered, for example.

Is the test framework single-threaded? Or might it run multiple fixtures on
different threads in the same process, at the same time?

Also note that we don't typically use scoped_ptr<> in statics in Chromium code,
since non-POD global variables cause the compiler to spit out initializer and
finalizer code, and that makes process startup & shutdown slower, amongst other
things. It can also lead to issues if any global statics end up depending on, or
are depended on, by other classes.

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

I think I started out with a more complicated 'first test case' than is
necessary.  I will update the fixture to remove the statics and we can revisit
this if the usage of the tool by the rest of the team shows there is a problem
here worth solving.

+
+  DISALLOW_COPY_AND_ASSIGN(AppRemotingConnectedClientTests);
+};
+
+}  // namespace test
+}  // namespace remoting
+
+#endif  // REMOTING_TEST_APP_REMOTING_CONNECTED_CLIENT_TESTS_H_