Retrieved Hostlist information from Directory service.

remoting/test/chromoting_host_list_fetcher_unittest.cc

Index: remoting/test/chromoting_host_list_fetcher_unittest.cc
diff --git a/remoting/test/chromoting_host_list_fetcher_unittest.cc b/remoting/test/chromoting_host_list_fetcher_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..af8098995425a728092fab277a9cae7101a78090
--- /dev/null
+++ b/remoting/test/chromoting_host_list_fetcher_unittest.cc
@@ -0,0 +1,207 @@
+// 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/chromoting_host_list_fetcher.h"
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/strings/stringprintf.h"
+#include "net/url_request/test_url_fetcher_factory.h"
+#include "remoting/test/chromoting_host_info.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+

[joedow, 2015/07/06 22:19:13]

remove extra newlines

[tonychun, 2015/07/08 03:12:15]

Done.

+
+namespace {
+// Used as a ChromotingHostListCallback for testing.
+void OnHostlistRetrieved(
+      base::Closure done_closure,
+      std::vector<remoting::test::ChromotingHostInfo>* hostlist,
+      std::vector<remoting::test::ChromotingHostInfo>* retrieved_hostlist) {

[joedow, 2015/07/06 22:19:14]

retrieved_hostlist should be const ref

[tonychun, 2015/07/08 03:12:15]

Done.

+  *hostlist = *retrieved_hostlist;
+
+  done_closure.Run();
+}
+
+const char kAccessTokenValue[] = "test_access_token_value";
+const char kChromotingHostListReadyResponse[] =
+"{"
+"  \"data\":{"
+"    \"kind\":\"chromoting#hostList\","
+"    \"items\":["
+"      {"
+"        \"tokenUrlPatterns\":["
+"          \"tokenUrlPattern_1A\","
+"          \"tokenUrlPattern_1B\","
+"          \"tokenUrlPattern_1C\""
+"        ],"
+"        \"kind\":\"chromoting#host\","
+"        \"hostId\":\"test_host_id_1\","
+"        \"hostName\":\"test_host_name_1\","
+"        \"publicKey\":\"test_public_key_1\","
+"        \"jabberId\":\"test_jabber_id_1\","
+"        \"createdTime\":\"test_created_time_1\","
+"        \"updatedTime\":\"test_updated_time_1\","
+"        \"status\":\"ONLINE\","
+"        \"hostOfflineReason\":\"\","
+"        \"hostVersion\":\"test_host_version_1\""
+"      },"
+"      {"
+"        \"kind\":\"chromoting#host\","
+"        \"hostId\":\"test_host_id_2\","
+"        \"hostName\":\"test_host_name_2\","
+"        \"publicKey\":\"test_public_key_2\","
+"        \"jabberId\":\"test_jabber_id_2\","
+"        \"createdTime\":\"test_created_time_2\","
+"        \"updatedTime\":\"test_updated_time_2\","
+"        \"status\":\"OFFLINE\","
+"        \"hostOfflineReason\":\"test_host_offline_reason_2\","
+"        \"hostVersion\":\"test_host_version_2\""
+"      }"
+"    ]"
+"  }"
+"}";

[joedow, 2015/07/06 22:19:13]

kChromotingHostListReadyResponse is a good start, but you probably also want
additional scenarios:

- empty items dict
data: { kind: items: {} } }

- no token url patterns (not sure if this is expected or an error)
data: { kind: items: { tokenUrlPatterns: [] ...details... } } }

- invalid status, possibly missing fields as is interesting

[tonychun, 2015/07/08 03:12:15]

Done.

+
+const char kChromotingHostListEmptyResponse[] = "{}";
+}  // namespace
+
+namespace remoting {
+namespace test {
+
+// Provides base functionality for the ChromotingHostListFetcher Tests below.
+// The FakeURLFetcherFactory allows us to override the response data and payload
+// for specified URLs. We use this to stub out network calls made by the
+// ChromotingHostListFetcher. This fixture also creates an IO MessageLoop, if

[joedow, 2015/07/06 22:19:14]

nit: it looks like it always creates a MessageLoop, this fixture doesn't check
to see if one exists.  That's fine to do, but the comment should be updated.

[tonychun, 2015/07/08 03:12:15]

Done.

+// necessary, for use by the ChromotingHostListFetcher.
+class ChromotingHostListFetcherTest : public ::testing::Test {
+ public:
+  ChromotingHostListFetcherTest() : url_fetcher_factory_(nullptr) {}
+  ~ChromotingHostListFetcherTest() override {}
+
+ protected:
+  // testing::Test interface.
+  void SetUp() override;
+
+  // Sets the HTTP status and data returned for a specified URL.
+  void SetFakeResponse(const GURL& url,
+                       const std::string& data,
+                       net::HttpStatusCode code,
+                       net::URLRequestStatus::Status status);
+
+ private:
+  net::FakeURLFetcherFactory url_fetcher_factory_;
+  scoped_ptr<base::MessageLoopForIO> message_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromotingHostListFetcherTest);
+};
+
+void ChromotingHostListFetcherTest::SetUp() {
+  DCHECK(!message_loop_);
+  message_loop_.reset(new base::MessageLoopForIO);
+
+  SetFakeResponse(GURL(kChromotingHostListProdRequestUrl),
+                  kChromotingHostListEmptyResponse, net::HTTP_NOT_FOUND,
+                  net::URLRequestStatus::FAILED);
+}
+
+void ChromotingHostListFetcherTest::SetFakeResponse(
+    const GURL& url,
+    const std::string& data,
+    net::HttpStatusCode code,
+    net::URLRequestStatus::Status status) {
+  url_fetcher_factory_.SetFakeResponse(url, data, code, status);
+}
+
+TEST_F(ChromotingHostListFetcherTest, RetrieveHostListFromProd) {
+  SetFakeResponse(GURL(kChromotingHostListProdRequestUrl),
+                  kChromotingHostListReadyResponse, net::HTTP_OK,
+                  net::URLRequestStatus::SUCCESS);
+
+  std::vector<ChromotingHostInfo> hostlist;
+
+  base::RunLoop run_loop;
+  HostlistCallback hostlist_fetch_callback = base::Bind(
+      &OnHostlistRetrieved,
+      run_loop.QuitClosure(),
+      &hostlist);
+
+  ChromotingHostListFetcher chromoting_host_list_fetcher;
+  bool request_started = chromoting_host_list_fetcher.RetrieveHostlist(
+      kAccessTokenValue, hostlist_fetch_callback);
+
+  run_loop.Run();
+
+  EXPECT_TRUE(request_started);
+  const unsigned int expectedHLSize = 2;

[joedow, 2015/07/06 22:19:14]

no camel case, this var should be called expected_hostlist_size

[tonychun, 2015/07/08 03:12:15]

Done.

+  EXPECT_EQ(hostlist.size(), expectedHLSize);
+
+  ChromotingHostInfo onlineChromotingHostInfo = hostlist.front();

[joedow, 2015/07/06 22:19:13]

Also, no camel case.  online_chromoting_host_info.

[joedow, 2015/07/06 22:19:14]

If you know the size, I would just use array indexing here instead of front/back

[tonychun, 2015/07/08 03:12:15]

Done.

[tonychun, 2015/07/08 03:12:15]

Done.

+  const unsigned int expectedTUPSize = 3;

[joedow, 2015/07/06 22:19:14]

expected_pattern_num?

[tonychun, 2015/07/08 03:12:15]

Done.

+  EXPECT_EQ(onlineChromotingHostInfo.tokenUrlPatterns.size(), expectedTUPSize);
+  EXPECT_TRUE(!onlineChromotingHostInfo.host_id.empty());

[joedow, 2015/07/06 22:19:14]

I think it's more readable when you think the string should be valid to use:
EXPECT_FALSE(my_fake_string.empty());

[tonychun, 2015/07/08 03:12:15]

Done.

+  EXPECT_TRUE(!onlineChromotingHostInfo.host_jid.empty());
+  EXPECT_TRUE(!onlineChromotingHostInfo.host_name.empty());
+  EXPECT_EQ(onlineChromotingHostInfo.status,
+      ChromotingHostStatus::kChromotingHostStatusOnline);
+  EXPECT_TRUE(onlineChromotingHostInfo.offline_reason.empty());
+  EXPECT_TRUE(!onlineChromotingHostInfo.public_key.empty());
+
+  ChromotingHostInfo offlineChromotingHostInfo = hostlist.back();

[joedow, 2015/07/06 22:19:14]

offline_chromoting_host_info

[tonychun, 2015/07/08 03:12:15]

Done.

+  EXPECT_TRUE(offlineChromotingHostInfo.tokenUrlPatterns.empty());
+  EXPECT_TRUE(!offlineChromotingHostInfo.host_id.empty());
+  EXPECT_TRUE(!offlineChromotingHostInfo.host_jid.empty());
+  EXPECT_TRUE(!offlineChromotingHostInfo.host_name.empty());
+  EXPECT_EQ(offlineChromotingHostInfo.status,
+      ChromotingHostStatus::kChromotingHostStatusOffline);
+  EXPECT_TRUE(!offlineChromotingHostInfo.offline_reason.empty());
+  EXPECT_TRUE(!offlineChromotingHostInfo.public_key.empty());
+}
+
+TEST_F(ChromotingHostListFetcherTest, RetrieveHostListInvalidEnvironment) {

[joedow, 2015/07/06 22:19:14]

The test names is "...InvalidEnvironment" but it looks like this is more of a
network error test.  Can you fix the name?

[tonychun, 2015/07/08 03:12:15]

Done.

+  base::RunLoop run_loop;
+
+  std::vector<ChromotingHostInfo> hostlist;
+
+  HostlistCallback hostlist_fetch_callback = base::Bind(
+      &OnHostlistRetrieved,
+      run_loop.QuitClosure(),
+      &hostlist);
+
+  ChromotingHostListFetcher chromoting_host_list_fetcher;
+  bool request_started = chromoting_host_list_fetcher.RetrieveHostlist(
+      kAccessTokenValue, hostlist_fetch_callback);
+
+  EXPECT_TRUE(request_started);
+  // If there was a network error retrieving the host list, then the host list
+  // should be empty.
+  EXPECT_TRUE(hostlist.empty());
+}
+

[joedow, 2015/07/06 22:19:14]

You should also add a test which makes two requests using the same hostlist
fetcher since it was written to allow that.

[tonychun, 2015/07/08 03:12:15]

Done.

+TEST_F(ChromotingHostListFetcherTest, RetrieveHostListEmptyResponse) {
+  SetFakeResponse(GURL(kChromotingHostListProdRequestUrl),
+                  kChromotingHostListEmptyResponse, net::HTTP_OK,
+                  net::URLRequestStatus::SUCCESS);
+
+  base::RunLoop run_loop;
+
+  std::vector<ChromotingHostInfo> hostlist;
+
+  HostlistCallback hostlist_fetch_callback = base::Bind(
+      &OnHostlistRetrieved,
+      run_loop.QuitClosure(),
+      &hostlist);
+
+  ChromotingHostListFetcher chromoting_host_list_fetcher;
+  bool request_started = chromoting_host_list_fetcher.RetrieveHostlist(
+      kAccessTokenValue, hostlist_fetch_callback);
+
+  EXPECT_TRUE(request_started);
+  // If we received an empty response, then none of the connection details
+  // should be populated.

[joedow, 2015/07/06 22:19:14]

"connection details"?  Copy/paste error?

[tonychun, 2015/07/08 03:12:15]

Done.

+  EXPECT_TRUE(hostlist.empty());
+}
+
+}  // namespace test
+}  // namespace remoting