Remove flakiness from url_info tests

This test was testing to be sure that timing was
correctly monitored during the lifetime of 
these UrlHostInfo instances.  The new code defensively
checks values, and when tehy appear to be out of
spec, it verifies that the test bot is indeed
running slowly, and exits.  This will usually
run the test as expected, but gracefully
exit when the bot is just too slow (or
has too much variance in execution speed)
to run the test carefully.

r=phajdan.jr
BUG=55169
Review URL: http://codereview.chromium.org/3431028

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@60962 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 47 insertions, 40 deletions

diff --git a/chrome/browser/net/url_info.cc b/chrome/browser/net/url_info.cc
index 7daf500..994cfe7 100644
--- a/chrome/browser/net/url_info.cc
+++ b/chrome/browser/net/url_info.cc
@@ -64,14 +64,20 @@ const TimeDelta UrlInfo::kNullDuration(TimeDelta::FromMilliseconds(-1));
 // has a TON of copies of the same domain name, so that we don't thrash the OS
 // to death.  Hopefully it is small enough that we're not hurting our cache hit
 // rate (i.e., we could always ask the OS).
-TimeDelta UrlInfo::kCacheExpirationDuration(TimeDelta::FromSeconds(5));
+TimeDelta UrlInfo::cache_expiration_duration_(TimeDelta::FromSeconds(5));
 
 const TimeDelta UrlInfo::kMaxNonNetworkDnsLookupDuration(
     TimeDelta::FromMilliseconds(15));
 
 // Used by test ONLY.  The value is otherwise constant.
+// static
 void UrlInfo::set_cache_expiration(TimeDelta time) {
-  kCacheExpirationDuration = time;
+  cache_expiration_duration_ = time;
+}
+
+// static
+TimeDelta UrlInfo::get_cache_expiration() {
+  return cache_expiration_duration_;
 }
 
 void UrlInfo::SetQueuedState(ResolutionMotivation motivation) {
@@ -160,7 +166,7 @@ bool UrlInfo::IsStillCached() const {
 
   TimeDelta time_since_resolution = TimeTicks::Now() - time_;
 
-  return time_since_resolution < kCacheExpirationDuration;
+  return time_since_resolution < cache_expiration_duration_;
 }
 
 void UrlInfo::DLogResultsStats(const char* message) const {
diff --git a/chrome/browser/net/url_info.h b/chrome/browser/net/url_info.h
index bd930ae..6b50f42 100644
--- a/chrome/browser/net/url_info.h
+++ b/chrome/browser/net/url_info.h
@@ -92,7 +92,9 @@ class UrlInfo {
   // on how recently we've done DNS prefetching for hostname.
   bool NeedsDnsUpdate();
 
+  // FOR TEST ONLY: The following access the otherwise constant values.
   static void set_cache_expiration(base::TimeDelta time);
+  static base::TimeDelta get_cache_expiration();
 
   // The prefetching lifecycle.
   void SetQueuedState(ResolutionMotivation motivation);
@@ -157,7 +159,7 @@ class UrlInfo {
   std::string GetAsciiMotivation() const;
 
   // The next declaration is non-const to facilitate testing.
-  static base::TimeDelta kCacheExpirationDuration;
+  static base::TimeDelta cache_expiration_duration_;
 
   // The current state of this instance.
   DnsProcessingState state_;
diff --git a/chrome/browser/net/url_info_unittest.cc b/chrome/browser/net/url_info_unittest.cc
index ea108ca..ea7d619 100644
--- a/chrome/browser/net/url_info_unittest.cc
+++ b/chrome/browser/net/url_info_unittest.cc
@@ -8,20 +8,24 @@
 #include <string>
 
 #include "base/platform_thread.h"
+#include "base/time.h"
 #include "chrome/browser/net/url_info.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::TimeDelta;
+using base::TimeTicks;
 
 namespace {
 
-class DnsHostInfoTest : public testing::Test {
+class UrlHostInfoTest : public testing::Test {
 };
 
 typedef chrome_browser_net::UrlInfo UrlInfo;
 
-// Flaky, http://crbug.com/55169.
-TEST(DnsHostInfoTest, FLAKY_StateChangeTest) {
+// Cycle throught the states held by a UrlInfo instance, and check to see that
+// states look reasonable as time ticks away.  If the test bots are too slow,
+// we'll just give up on this test and exit from it.
+TEST(UrlHostInfoTest, StateChangeTest) {
   UrlInfo info_practice, info;
   GURL url1("http://domain1.com:80"), url2("https://domain2.com:443");
 
@@ -32,54 +36,49 @@ TEST(DnsHostInfoTest, FLAKY_StateChangeTest) {
   info_practice.SetQueuedState(UrlInfo::UNIT_TEST_MOTIVATED);
   info_practice.SetAssignedState();
   info_practice.SetFoundState();
-  PlatformThread::Sleep(500);  // Allow time for DLLs to fully load.
+
+  // Start test with actual (long/default) expiration time intact.
 
   // Complete the construction of real test object.
   info.SetUrl(url1);
-
   EXPECT_TRUE(info.NeedsDnsUpdate()) << "error in construction state";
   info.SetQueuedState(UrlInfo::UNIT_TEST_MOTIVATED);
-  EXPECT_FALSE(info.NeedsDnsUpdate())
-    << "update needed after being queued";
+  EXPECT_FALSE(info.NeedsDnsUpdate()) << "update needed after being queued";
   info.SetAssignedState();
-  EXPECT_FALSE(info.NeedsDnsUpdate());
+  EXPECT_FALSE(info.NeedsDnsUpdate())  << "update needed during resolution";
+  base::TimeTicks before_resolution_complete = TimeTicks::Now();
   info.SetFoundState();
-  EXPECT_FALSE(info.NeedsDnsUpdate())
-    << "default expiration time is TOOOOO short";
-
-  // Note that time from ASSIGNED to FOUND was VERY short (probably 0ms), so the
-  // object should conclude that no network activity was needed.  As a result,
-  // the required time till expiration will be halved (guessing that we were
-  // half way through having the cache expire when we did the lookup.
-  EXPECT_LT(info.resolve_duration().InMilliseconds(),
-    UrlInfo::kMaxNonNetworkDnsLookupDuration.InMilliseconds())
-    << "Non-net time is set too low";
-
-  info.set_cache_expiration(TimeDelta::FromMilliseconds(300));
-  EXPECT_FALSE(info.NeedsDnsUpdate()) << "expiration time not honored";
-  PlatformThread::Sleep(80);  // Not enough time to pass our 300ms mark.
-  EXPECT_FALSE(info.NeedsDnsUpdate()) << "expiration time not honored";
+  // "Immediately" check to see if we need an update yet (we shouldn't).
+  if (info.NeedsDnsUpdate()) {
+    // The test bot must be really slow, so we can verify that.
+    EXPECT_GT((TimeTicks::Now() - before_resolution_complete).InMilliseconds(),
+              UrlInfo::get_cache_expiration().InMilliseconds());
+    return;  // Lets punt here, the test bot is too slow.
+  }
+
+  // Run similar test with a shortened expiration, so we can trigger it.
+  const int kMockExpirationTime(300);  // Vastly reduced expiration time.
+  info.set_cache_expiration(TimeDelta::FromMilliseconds(kMockExpirationTime));
 
   // That was a nice life when the object was found.... but next time it won't
   // be found.  We'll sleep for a while, and then come back with not-found.
   info.SetQueuedState(UrlInfo::UNIT_TEST_MOTIVATED);
+  EXPECT_FALSE(info.NeedsDnsUpdate());
   info.SetAssignedState();
   EXPECT_FALSE(info.NeedsDnsUpdate());
   // Greater than minimal expected network latency on DNS lookup.
   PlatformThread::Sleep(25);
+  before_resolution_complete = TimeTicks::Now();
   info.SetNoSuchNameState();
-  EXPECT_FALSE(info.NeedsDnsUpdate())
-    << "default expiration time is TOOOOO short";
-
-  // Note that now we'll actually utilize an expiration of 300ms,
-  // since there was detected network activity time during lookup.
-  // We're assuming the caching just started with our lookup.
-  PlatformThread::Sleep(80);  // Not enough time to pass our 300ms mark.
-  EXPECT_FALSE(info.NeedsDnsUpdate()) << "expiration time not honored";
-  // Still not past our 300ms mark (only about 4+2ms)
-  PlatformThread::Sleep(80);
-  EXPECT_FALSE(info.NeedsDnsUpdate()) << "expiration time not honored";
-  PlatformThread::Sleep(150);
+  // "Immediately" check to see if we need an update yet (we shouldn't).
+  if (info.NeedsDnsUpdate()) {
+    // The test bot must be really slow, so we can verify that.
+    EXPECT_GT((TimeTicks::Now() - before_resolution_complete).InMilliseconds(),
+              kMockExpirationTime);
+    return;
+  }
+  // Wait over 300ms, so it should definately be considered out of cache.
+  PlatformThread::Sleep(kMockExpirationTime + 20);
   EXPECT_TRUE(info.NeedsDnsUpdate()) << "expiration time not honored";
 }
 
@@ -92,7 +91,7 @@ TEST(DnsHostInfoTest, FLAKY_StateChangeTest) {
 // getting sent to a resolver thread) and reset our state to what it was before
 // the corresponding name was put in the work_queue_.  This test drives through
 // the state transitions used in such congestion handling.
-TEST(DnsHostInfoTest, CongestionResetStateTest) {
+TEST(UrlHostInfoTest, CongestionResetStateTest) {
   UrlInfo info;
   GURL url("http://domain1.com:80");