[Clean-up] Replace the final straggler use of MockTimeProvider with Clock.

components/search_engines/template_url_service_unittest.cc

 // Copyright (c) 2012 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 "components/search_engines/template_url_service.h"
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/run_loop.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/cancelable_task_tracker.h"
-#include "base/test/mock_time_provider.h"
+#include "base/test/simple_test_clock.h"
 #include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/search_engines/template_url_service_test_util.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/search_engines/keyword_web_data_service.h"
 #include "components/search_engines/search_host_to_urls_map.h"
 #include "components/search_engines/search_terms_data.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_prepopulate_data.h"
-#include "components/search_engines/template_url_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::ASCIIToUTF16;
 using base::Time;
 using base::TimeDelta;
-using ::testing::Return;
-using ::testing::StrictMock;
 
 namespace {
 
 // QueryHistoryCallbackImpl ---------------------------------------------------
 
 struct QueryHistoryCallbackImpl {
   QueryHistoryCallbackImpl() : success(false) {}
 
   void Callback(bool success,
                 const history::URLRow& row,
@@ skipping 174 matching lines @@
   ASSERT_EQ(expected.keyword(), actual.keyword());
   ASSERT_EQ(expected.url(), actual.url());
   ASSERT_EQ(expected.suggestions_url(), actual.suggestions_url());
   ASSERT_EQ(expected.favicon_url(), actual.favicon_url());
   ASSERT_EQ(expected.alternate_urls(), actual.alternate_urls());
   ASSERT_EQ(expected.show_in_default_list(), actual.show_in_default_list());
   ASSERT_EQ(expected.safe_for_autoreplace(), actual.safe_for_autoreplace());
   ASSERT_EQ(expected.input_encodings(), actual.input_encodings());
   ASSERT_EQ(expected.id(), actual.id());
   ASSERT_EQ(expected.date_created(), actual.date_created());
-  ASSERT_EQ(expected.last_modified(), actual.last_modified());
+  // Note that times are stored with a granularity of one second.

[Peter Kasting, 2015/01/22 00:00:23]

Doesn't this mean the assert would have to be LE instead of LT?  In case we
ticked over to the next second before this ASSERT?

Same comment below.  (I wonder if the three times you use this make it worth a
helper function.)

[Ilya Sherman, 2015/01/22 02:38:22]

The issue is not that we might tick over a second.  Rather, the issue is a loss
of precision: If we started with a time that was 36.077 seconds, we end up with
a time that is 36.000 seconds.  The other option is to set expected times that
are aligned to precise seconds, as the tests previously did.

I agree that a helper function is probably more readable; done.

[Peter Kasting, 2015/01/22 17:54:22]

Right, but what I'm worried about is, if the start time is 36.499 seconds and
the end time is 36.501 seconds, we'll get 36 and 37 seconds and the total time
will be one second.  Or am I mistaken about whether this can happen?

[Ilya Sherman, 2015/01/22 23:13:11]

That's not possible -- the test Clock will always return the same value for
calls to Now() (unless SetNow() is called again).

+  ASSERT_LT((expected.last_modified() - actual.last_modified()).magnitude(),

[tfarina, 2015/01/16 05:01:39]

Looks like this accounts for the small difference I was seeing when this check
was failing for me.

+            base::TimeDelta::FromSeconds(1));
   ASSERT_EQ(expected.sync_guid(), actual.sync_guid());
   ASSERT_EQ(expected.search_terms_replacement_key(),
             actual.search_terms_replacement_key());
 }
 
 TemplateURL* TemplateURLServiceTest::CreatePreloadedTemplateURL(
     bool safe_for_autoreplace,
     int prepopulate_id) {
   TemplateURLData data;
   data.short_name = ASCIIToUTF16("unittest");
@@ skipping 57 matching lines @@
   test_util()->ResetModel(true);
   ASSERT_EQ(initial_count + 1, model()->GetTemplateURLs().size());
   TemplateURL* loaded_url =
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword"));
   ASSERT_TRUE(loaded_url != NULL);
   AssertEquals(*cloned_url, *loaded_url);
   ASSERT_TRUE(model()->CanReplaceKeyword(ASCIIToUTF16("keyword"), GURL(),
                                          NULL));
 
   // We expect the last_modified time to be updated to the present time on an
-  // explicit reset. We have to set up the expectation here because ResetModel
-  // resets the TimeProvider in the TemplateURLService.
-  StrictMock<base::MockTimeProvider> mock_time;
-  model()->set_time_provider(&base::MockTimeProvider::StaticNow);
-  EXPECT_CALL(mock_time, Now()).WillOnce(Return(base::Time::FromDoubleT(1337)));
+  // explicit reset.
+  base::Time now = base::Time::Now();
+  scoped_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock);

[tfarina, 2015/01/16 04:28:51]

Doesn't have crashed for you? I mean, not this line, but later on, scoped_ptr
will raise a check.

[Ilya Sherman, 2015/01/16 04:39:18]

This doesn't crash, because the ownership is transferred to the model.  I'm
guessing you had a crash because both the model and the test function tried to
maintain ownership of the clock.

+  clock->SetNow(now);
+  model()->set_clock(clock.Pass());
 
   // Mutate an element and verify it succeeded.
   model()->ResetTemplateURL(loaded_url, ASCIIToUTF16("a"), ASCIIToUTF16("b"),
                             "c");
   ASSERT_EQ(ASCIIToUTF16("a"), loaded_url->short_name());
   ASSERT_EQ(ASCIIToUTF16("b"), loaded_url->keyword());
   ASSERT_EQ("c", loaded_url->url());
   ASSERT_FALSE(loaded_url->safe_for_autoreplace());
   ASSERT_TRUE(model()->CanReplaceKeyword(ASCIIToUTF16("keyword"), GURL(),
                                          NULL));
   ASSERT_FALSE(model()->CanReplaceKeyword(ASCIIToUTF16("b"), GURL(), NULL));
   cloned_url.reset(new TemplateURL(loaded_url->data()));
   base::RunLoop().RunUntilIdle();
   test_util()->ResetModel(true);
   ASSERT_EQ(initial_count + 1, model()->GetTemplateURLs().size());
   loaded_url = model()->GetTemplateURLForKeyword(ASCIIToUTF16("b"));
   ASSERT_TRUE(loaded_url != NULL);
   AssertEquals(*cloned_url, *loaded_url);

[tfarina, 2015/01/16 04:28:51]

This didn't pass for me locally.

[Ilya Sherman, 2015/01/16 04:39:18]

Note that I modified the AssertEquals implementation.  The simpler option would
be to use a Time value that does not change when rounded to the nearest second.

   // We changed a TemplateURL in the service, so ensure that the time was
-  // updated.
-  ASSERT_EQ(base::Time::FromDoubleT(1337), loaded_url->last_modified());
+  // updated. Note that times are stored with a granularity of one second.
+  ASSERT_LT((now - loaded_url->last_modified()).magnitude(),
+            base::TimeDelta::FromSeconds(1));
 
   // Remove an element and verify it succeeded.
   model()->Remove(loaded_url);
   VerifyObserverCount(1);
   test_util()->ResetModel(true);
   ASSERT_EQ(initial_count, model()->GetTemplateURLs().size());
   EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("b")) == NULL);
 }
 
 TEST_F(TemplateURLServiceTest, AddSameKeyword) {
@@ skipping 269 matching lines @@
   data.SetURL("http://www.google.com/foo/bar");
   data.favicon_url = GURL("http://favicon.url");
   data.date_created = Time::FromTimeT(100);
   data.last_modified = Time::FromTimeT(100);
   TemplateURL* t_url = new TemplateURL(data);
   model()->Add(t_url);
 
   VerifyObserverCount(1);
   base::RunLoop().RunUntilIdle();
 
-  StrictMock<base::MockTimeProvider> mock_time;
-  model()->set_time_provider(&base::MockTimeProvider::StaticNow);
-  EXPECT_CALL(mock_time, Now()).WillOnce(Return(base::Time::FromDoubleT(1337)));
-
   // Reset the short name, keyword, url and make sure it takes.
   const base::string16 new_short_name(ASCIIToUTF16("a"));
   const base::string16 new_keyword(ASCIIToUTF16("b"));
   const std::string new_url("c");
   model()->ResetTemplateURL(t_url, new_short_name, new_keyword, new_url);
   ASSERT_EQ(new_short_name, t_url->short_name());
   ASSERT_EQ(new_keyword, t_url->keyword());
   ASSERT_EQ(new_url, t_url->url());
 
   // Make sure the mappings in the model were updated.
   ASSERT_EQ(t_url, model()->GetTemplateURLForKeyword(new_keyword));
   ASSERT_TRUE(
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")) == NULL);
 
   scoped_ptr<TemplateURL> cloned_url(new TemplateURL(t_url->data()));
 
+  base::Time now = base::Time::Now();

[tfarina, 2015/01/16 05:01:39]

Have you had to move this down for some reason? Or just to be closer to the
assert below? Because there is and ResetTemplateURL call above that sets
last_modifed too.

[Ilya Sherman, 2015/01/16 05:11:07]

That's a good question.  I took your CL as a starting point, and you had moved
this down, so I left it where you'd put it.  I'm not sure why the test passes,
given the ResetTemplateURL call above, but it does pass.

[Peter Kasting, 2015/01/22 00:00:23]

Hmm.  It seems like it ought to fail to me, since I'd expect last_modified to
get updated at the ResetTemplateURL call.  Is the ResetModel call below doing
this?  If so that's probably a bug that we should fix.

[Ilya Sherman, 2015/01/22 02:38:22]

I'm not sure what's causing the test to continue to pass, but it does.  I've
restored the original position of the code for now.  If ResetModel() is causing
last_modified to get updated, then that bug is outside the intended scope of
this CL.

[Peter Kasting, 2015/01/22 17:54:22]

Yeah, I wasn't so much asking you to fix it as hoping you'd glance at why the
failure is occurring and file a bug against me if it looks wrong.  I guess
that's somewhat passive-aggressive/lazy :)

[Ilya Sherman, 2015/01/22 23:13:11]

Fair enough.  I didn't investigate deeply enough to know how to file a
meaningful bug, sorry.  (I wrote this CL quickly over SSH, and I don't know of a
non-PITA way to run the tests over SSH, so I wanted to minimize the number of
times that I ran the tests.)

+  scoped_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock);
+  clock->SetNow(now);
+  model()->set_clock(clock.Pass());
+
   // Reload the model from the database and make sure the change took.
   test_util()->ResetModel(true);
   EXPECT_EQ(initial_count + 1, model()->GetTemplateURLs().size());
   const TemplateURL* read_url = model()->GetTemplateURLForKeyword(new_keyword);
   ASSERT_TRUE(read_url);
   AssertEquals(*cloned_url, *read_url);
-  ASSERT_EQ(base::Time::FromDoubleT(1337), read_url->last_modified());
+  // Note that times are stored with a granularity of one second.
+  ASSERT_LT((now - read_url->last_modified()).magnitude(),
+            base::TimeDelta::FromSeconds(1));
 }
 
 TEST_F(TemplateURLServiceTest, DefaultSearchProvider) {
   // Add a new TemplateURL.
   test_util()->VerifyLoad();
   const size_t initial_count = model()->GetTemplateURLs().size();
   TemplateURL* t_url = AddKeywordWithDate(
       "name1", "key1", "http://foo1/{searchTerms}", "http://sugg1",
       std::string(), "http://icon1", true, "UTF-8;UTF-16", Time(), Time());
   test_util()->ResetObserverCount();
@@ skipping 818 matching lines @@
   scoped_ptr<TemplateURL::AssociatedExtensionInfo> extension_info(
       new TemplateURL::AssociatedExtensionInfo(
           TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION, "ext1"));
   extension_info->wants_to_be_default_engine = true;
   model()->AddExtensionControlledTURL(ext_dse, extension_info.Pass());
   EXPECT_EQ(ext_dse, model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext1")));
   EXPECT_TRUE(model()->is_default_search_managed());
   actual_managed_default = model()->GetDefaultSearchProvider();
   ExpectSimilar(expected_managed_default.get(), actual_managed_default);
 }