Fixing the Chromoting Test Driver host online retry logic

remoting/test/chromoting_test_driver_environment.cc

 // 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_test_driver_environment.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 12 matching lines @@
 
 ChromotingTestDriverEnvironment::EnvironmentOptions::EnvironmentOptions() {
 }
 
 ChromotingTestDriverEnvironment::EnvironmentOptions::~EnvironmentOptions() {
 }
 
 ChromotingTestDriverEnvironment::ChromotingTestDriverEnvironment(
     const EnvironmentOptions& options)
     : host_name_(options.host_name),
+      host_jid_(options.host_jid),
       user_name_(options.user_name),
       pin_(options.pin),
       refresh_token_file_path_(options.refresh_token_file_path),
       test_access_token_fetcher_(nullptr),
       test_refresh_token_store_(nullptr),
       test_host_list_fetcher_(nullptr) {
   DCHECK(!user_name_.empty());
   DCHECK(!host_name_.empty());
 }
 
@@ skipping 27 matching lines @@
     VLOG(2) << "No refresh token stored for " << user_name_;
 
     if (auth_code.empty()) {
       // No token and no Auth code means no service connectivity, bail!
       LOG(ERROR) << "Cannot retrieve an access token without a stored refresh"
                  << " token on disk or an auth_code passed into the tool";
       return false;
     }
   }
 
-  if (!RetrieveAccessToken(auth_code) || !RetrieveHostList()) {
-    // If we cannot retrieve an access token or a host list, then nothing is
-    // going to work. We should let the caller know that our object is not ready
-    // to be used.
+  if (!RetrieveAccessToken(auth_code)) {
+    // If we cannot retrieve an access token, then nothing is going to work.
+    // Let the caller know that our object is not ready to be used.
     return false;
   }
 
   return true;
 }
 
 void ChromotingTestDriverEnvironment::DisplayHostList() {
   const char kHostAvailabilityFormatString[] = "%-45s%-15s%-35s";
 
   LOG(INFO) << base::StringPrintf(kHostAvailabilityFormatString,
@@ skipping 14 matching lines @@
 
     LOG(INFO) << base::StringPrintf(
         kHostAvailabilityFormatString, host_info.host_name.c_str(),
         status.c_str(), host_info.host_jid.c_str());
   }
 }
 
 bool ChromotingTestDriverEnvironment::WaitForHostOnline(
     const std::string& host_jid,
     const std::string& host_name) {
+  if (host_list_.empty()) {
+    RetrieveHostList();
+  }
+
   // Refresh the |host_list_| periodically to check if expected JID is online.
-  const int kMaxIterations = 12;
+  const base::TimeDelta kTotalTimeInSeconds = base::TimeDelta::FromSeconds(60);
   const base::TimeDelta kSleepTimeInSeconds = base::TimeDelta::FromSeconds(5);
+  const int kMaxIterations = kTotalTimeInSeconds / kSleepTimeInSeconds;
+
   int num_iterations = 0;
   while (num_iterations < kMaxIterations) {
-    if (IsHostOnline(host_jid, host_name)) {
+    if (host_info_.IsReadyForConnection()) {
       if (num_iterations > 0) {
-        VLOG(0) << "Host came online after "
+        VLOG(0) << "Host online after: "
                 << num_iterations * kSleepTimeInSeconds.InSeconds()
                 << " seconds.";
       }
       return true;
     }
+
     // Wait a while before refreshing host list.
     base::PlatformThread::Sleep(kSleepTimeInSeconds);
     RefreshHostList();
     ++num_iterations;
   }
+
   LOG(ERROR) << "Host with JID '" << host_jid << "' still not online after "
              << num_iterations * kSleepTimeInSeconds.InSeconds() << " seconds.";
   return false;
 }
 
-bool ChromotingTestDriverEnvironment::IsHostOnline(
-    const std::string host_jid,
-    const std::string host_name) const {
-  for (const HostInfo& host_info : host_list_) {
-    if (host_jid == host_info.host_jid && host_name == host_info.host_name) {
-      if (host_info.status == kHostStatusOnline) {
-        return true;
-      } else {
-        LOG(WARNING) << "Host '" << host_name << "' with JID '" << host_jid
-                     << "' not online.";
-        return false;
-      }
-    }
-  }
-  return false;
-}
-
 void ChromotingTestDriverEnvironment::SetAccessTokenFetcherForTest(
     AccessTokenFetcher* access_token_fetcher) {
   DCHECK(access_token_fetcher);
 
   test_access_token_fetcher_ = access_token_fetcher;
 }
 
 void ChromotingTestDriverEnvironment::SetRefreshTokenStoreForTest(
     RefreshTokenStore* refresh_token_store) {
   DCHECK(refresh_token_store);
 
   test_refresh_token_store_ = refresh_token_store;
 }
 
 void ChromotingTestDriverEnvironment::SetHostListFetcherForTest(
     HostListFetcher* host_list_fetcher) {
   DCHECK(host_list_fetcher);
 
   test_host_list_fetcher_ = host_list_fetcher;
 }
 
+void ChromotingTestDriverEnvironment::SetHostNameForTest(
+    const std::string& host_name) {
+  host_name_ = host_name;
+}
+
+void ChromotingTestDriverEnvironment::SetHostJidForTest(
+    const std::string& host_jid) {
+  host_jid_ = host_jid;
+}
+
 void ChromotingTestDriverEnvironment::TearDown() {
   // Letting the MessageLoop tear down during the test destructor results in
   // errors after test completion, when the MessageLoop dtor touches the
   // registered AtExitManager. The AtExitManager is torn down before the test
   // destructor is executed, so we tear down the MessageLoop here, while it is
   // still valid.
   message_loop_.reset();
 }
 
 bool ChromotingTestDriverEnvironment::RetrieveAccessToken(
@@ skipping 80 matching lines @@
 
 bool ChromotingTestDriverEnvironment::RefreshHostList() {
   host_list_.clear();
 
   return RetrieveHostList();
 }
 
 bool ChromotingTestDriverEnvironment::RetrieveHostList() {
   base::RunLoop run_loop;
 
+  // Clear the previous host info.
   host_info_ = HostInfo();
 
   // If a unit test has set |test_host_list_fetcher_| then we should use it
   // below.  Note that we do not want to destroy the test object at the end of
   // the function which is why we have the dance below.
   scoped_ptr<HostListFetcher> temporary_host_list_fetcher;
   HostListFetcher* host_list_fetcher = test_host_list_fetcher_;
   if (!host_list_fetcher) {
     temporary_host_list_fetcher.reset(new HostListFetcher());
     host_list_fetcher = temporary_host_list_fetcher.get();
   }
 
   remoting::test::HostListFetcher::HostlistCallback host_list_callback =
       base::Bind(&ChromotingTestDriverEnvironment::OnHostListRetrieved,
                  base::Unretained(this), run_loop.QuitClosure());
 
   host_list_fetcher->RetrieveHostlist(access_token_, host_list_callback);
 
   run_loop.Run();
 
   if (host_list_.empty()) {
     // Note: Access token may have expired, but it is unlikely.
     LOG(ERROR) << "Retrieved host list is empty.\n"
                << "Does the account have hosts set up?";
     return false;
   }
 
-  // If the host or command line parameters are not setup correctly, we want to
-  // let the user fix the issue before continuing.
-  bool found_host_name = false;
-  auto host_info_iter = std::find_if(host_list_.begin(), host_list_.end(),
-      [this, &found_host_name](const remoting::test::HostInfo& host_info) {
-          if (host_info.host_name == host_name_) {
-            found_host_name = true;
-            return host_info.IsReadyForConnection();
-          }
-          return false;
-      });
-  if (host_info_iter == host_list_.end()) {
-    if (found_host_name) {
-      LOG(ERROR) << this->host_name_ << " is not ready to connect.";
-    } else {
-      LOG(ERROR) << this->host_name_ << " was not found in the host list.";
+  DisplayHostList();
+  for (HostInfo& host_info : host_list_) {
+    // The JID is optional so we consider an empty string to be a '*' match.
+    bool host_jid_match =
+        host_jid_.empty() || (host_jid_ == host_info.host_jid);
+    bool host_name_match = host_name_ == host_info.host_name;
+
+    if (host_name_match && host_jid_match) {
+      host_info_ = host_info;
+
+      if (host_info.IsReadyForConnection()) {
+        return true;
+      } else {
+        LOG(WARNING) << "Host '" << host_name_ << "' with JID '" << host_jid_
+                     << "' not online.";
+        return false;
+      }
     }
-    DisplayHostList();
-    return false;
   }
-
-  host_info_ = *host_info_iter;
-
-  return true;
+  LOG(WARNING) << "Host '" << host_name_ << "' with JID '" << host_jid_
+               << "' not found in host list.";
+  return false;
 }
 
 void ChromotingTestDriverEnvironment::OnHostListRetrieved(
     base::Closure done_closure,
     const std::vector<HostInfo>& retrieved_host_list) {
   VLOG(1) << "OnHostListRetrieved() Called";
 
   host_list_ = retrieved_host_list;
 
   done_closure.Run();
 }
 
 }  // namespace test
 }  // namespace remoting