Cleaning up test only variables.

components/omnibox/browser/in_memory_url_index_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 <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <fstream>
 
@@ skipping 70 matching lines @@
   if ((cursor_position != kInvalid) &&
       (cursor_position < lower_string->length()) && (cursor_position > 0)) {
     lower_string->insert(cursor_position, base::ASCIIToUTF16(" "));
   }
 
   *lower_terms = base::SplitString(*lower_string, base::kWhitespaceUTF16,
                                    base::KEEP_WHITESPACE,
                                    base::SPLIT_WANT_NONEMPTY);
 }
 
+constexpr size_t kItemsToScoreLimit = 500;

[dyaroshev, 2017/02/22 22:21:51]

Make constants local.

[dyaroshev, 2017/02/24 01:27:03]

Done.

+constexpr int kLowVisitCount = 20;
+constexpr int kHighVisitCount = 200;
+constexpr int kLowTypedCount = 2;
+constexpr int kHighTypedCount = 100;
+
+constexpr int kMinRowId = 5000;
+
+base::Time OldVisitTime() {

[Peter Kasting, 2017/02/23 01:04:45]

Nit: I would make these const temps in the caller side instead of helper
functions, for a couple small reasons.

One is that it's more readable to have the definition right there in code than
to look up here for it.  In the case of RecentVisitTime(), there's only one
caller, so we wouldn't even duplicate code.

Another is that it's not clear whether these values are arbitrary, and if they
need to be the same across all callers, or are just generic "any old visit". 
Placing the definitions in the callers makes it more clear the tests are
independent of each other.

The third reason is that this will let you call Now() once and store the result
in a temp instead of recomputing it repeatedly, which is an efficiency win.

[dyaroshev, 2017/02/23 01:41:26]

I think think that calling now is good because we get different timings for
different results.

[dyaroshev, 2017/02/24 01:27:03]

I've done smth. Is it ok?

+  return base::Time::Now() - base::TimeDelta::FromDays(15);
+};
+base::Time RecentVisitTime() {
+  return base::Time::Now() - base::TimeDelta::FromDays(2);
+};
+
+HistoryIDSet FillHistoryIDSet(HistoryID min, HistoryID max) {
+  HistoryIDSet res;
+  // All ids are inserted in the end so the implicit hint would work.
+  for (HistoryID id = min; id < max; ++id)
+    res.insert(id);
+  return res;
+}
+
 }  // namespace
 
 // -----------------------------------------------------------------------------
 
 // Observer class so the unit tests can wait while the cache is being saved.
 class CacheFileSaverObserver : public InMemoryURLIndex::SaveCacheObserver {
  public:
   explicit CacheFileSaverObserver(const base::Closure& task);
 
   bool succeeded() { return succeeded_; }
@@ skipping 52 matching lines @@
   base::CancelableTaskTracker* GetPrivateDataTracker() const;
   void ClearPrivateData();
   void set_history_dir(const base::FilePath& dir_path);
   bool GetCacheFilePath(base::FilePath* file_path) const;
   void PostRestoreFromCacheFileTask();
   void PostSaveToCacheFileTask();
   const SchemeSet& scheme_whitelist();
 
   // Pass-through functions to simplify our friendship with URLIndexPrivateData.
   bool UpdateURL(const history::URLRow& row);
+  void AddHistoryEntry(history::URLID row_id,
+                       int typed_count,
+                       int visit_count,
+                       base::Time last_visit);
   bool DeleteURL(const GURL& url);
 
   // Data verification helper functions.
   void ExpectPrivateDataNotEmpty(const URLIndexPrivateData& data);
   void ExpectPrivateDataEmpty(const URLIndexPrivateData& data);
   void ExpectPrivateDataEqual(const URLIndexPrivateData& expected,
                               const URLIndexPrivateData& actual);
 
   base::MessageLoop message_loop_;
   base::SequencedWorkerPoolOwner pool_owner_;
@@ skipping 46 matching lines @@
 const SchemeSet& InMemoryURLIndexTest::scheme_whitelist() {
   return url_index_->scheme_whitelist();
 }
 
 bool InMemoryURLIndexTest::UpdateURL(const history::URLRow& row) {
   return GetPrivateData()->UpdateURL(
       history_service_.get(), row, url_index_->scheme_whitelist_,
       GetPrivateDataTracker());
 }
 
+void InMemoryURLIndexTest::AddHistoryEntry(history::URLID row_id,
+                                           int typed_count,
+                                           int visit_count,
+                                           base::Time last_visit) {
+  history::URLRow new_row(
+      GURL("http://www.fake_url" + std::to_string(row_id) + ".com"), row_id);
+  new_row.set_visit_count(visit_count);
+  new_row.set_typed_count(typed_count);
+  new_row.set_last_visit(last_visit);
+  UpdateURL(std::move(new_row));
+};
+
 bool InMemoryURLIndexTest::DeleteURL(const GURL& url) {
   return GetPrivateData()->DeleteURL(url);
 }
 
 void InMemoryURLIndexTest::SetUp() {
   // We cannot access the database until the backend has been loaded.
   if (history_dir_.CreateUniqueTempDir())
     history_service_ =
         history::CreateHistoryService(history_dir_.GetPath(), true);
   ASSERT_TRUE(history_service_);
@@ skipping 464 matching lines @@
       ASCIIToUTF16("atdmt view"), base::string16::npos, kMaxMatches);
   EXPECT_EQ(1U, matches.size());
   matches = url_index_->HistoryItemsForTerms(ASCIIToUTF16("atdmt.view"),
                                              base::string16::npos, kMaxMatches);
   EXPECT_EQ(0U, matches.size());
   matches = url_index_->HistoryItemsForTerms(ASCIIToUTF16("view.atdmt"),
                                              base::string16::npos, kMaxMatches);
   EXPECT_EQ(1U, matches.size());
 }
 
+TEST_F(InMemoryURLIndexTest, DontTrimMoreThanLimit) {

[Peter Kasting, 2017/02/23 01:04:45]

Nit: Especially for the second test you add, but really for both of these,
please give a descriptive comment sentence describing what the test is testing.

[dyaroshev, 2017/02/24 01:27:03]

Done?

+  history::URLID row_id = kMinRowId;
+  AddHistoryEntry(row_id, kLowTypedCount, kLowVisitCount, OldVisitTime());
+  for (++row_id; size_t(row_id) < kItemsToScoreLimit - 1; ++row_id) {

[Peter Kasting, 2017/02/23 01:04:45]

I don't think this works right?  kMinRowId = 5000 and kItemsToScoreLimit = 500. 
I think you meant to compare against kMinRowId + kItemsToScoreLimit or
something, or the loop won't execute.

Nit: Don't cast to size_t with this syntax; use a static_cast (2 places).  No
{}.

[dyaroshev, 2017/02/24 01:27:03]

Not applicable.

+    AddHistoryEntry(row_id, kLowTypedCount, kLowVisitCount, OldVisitTime());
+  }

[dyaroshev, 2017/02/22 22:21:51]

One extra addition is unnecessary.

[Peter Kasting, 2017/02/23 01:04:45]

Not sure what this is saying.  Is it explaining the "- 1" in the loop condition
above?  Because I don't really get that.

[dyaroshev, 2017/02/23 01:41:26]

It's more for me - to avoid extra patches, if I see smth I want fixed, I leave a
comment and then fix with your issues. Here I'm calling AddHistory... 2 times:
one before the loop, one after - this is unnecessary.  

[Peter Kasting, 2017/02/23 01:46:06]

OK.  I still don't really get the "- 1" in the loop conditional then.

[dyaroshev, 2017/02/24 01:27:03]

Not applicable.

+
+  auto history_id_set = FillHistoryIDSet(kMinRowId, row_id);

[dyaroshev, 2017/02/22 22:21:52]

Check that all ids are in

+
+  EXPECT_FALSE(GetPrivateData()->TrimHistoryIdsPool(&history_id_set));
+  EXPECT_EQ(size_t(row_id - kMinRowId), history_id_set.size());
+}
+
+TEST_F(InMemoryURLIndexTest, TrimByProrities) {
+  int expected_worst = 1;

[Peter Kasting, 2017/02/23 01:04:45]

Nit: I can't instantly tell what this code is doing beyond the general "expect
that the right number of things got trimmed".  Add comments above blocks of code
giving the high level summary of what a large block does.

[dyaroshev, 2017/02/24 01:27:03]

Rewrote the test.

+  int expected_recent = 2;
+  int expected_visit_count = 3;
+  int expected_typed_count = kItemsToScoreLimit - expected_visit_count -
+                             expected_recent - expected_worst;
+
+  const history::URLID last_high_typed_count = kMinRowId + expected_typed_count;
+  const history::URLID last_high_visit_count =
+      last_high_typed_count + expected_visit_count;
+  const history::URLID last_recent = last_high_visit_count + expected_recent;
+  const history::URLID last_worst = last_recent + kItemsToScoreLimit;
+
+  history::URLID row_id = kMinRowId;
+  for (; row_id < last_high_typed_count; ++row_id)
+    AddHistoryEntry(row_id, kHighTypedCount, kLowVisitCount, OldVisitTime());
+
+  for (; row_id < last_high_visit_count; ++row_id)
+    AddHistoryEntry(row_id, kLowTypedCount, kHighVisitCount, OldVisitTime());
+
+  for (; row_id < last_recent; ++row_id)
+    AddHistoryEntry(row_id, kLowTypedCount, kLowVisitCount, RecentVisitTime());
+
+  for (; row_id < last_worst; ++row_id)
+    AddHistoryEntry(row_id, kLowTypedCount, kLowVisitCount, OldVisitTime());
+
+  auto history_id_set = FillHistoryIDSet(kMinRowId, row_id);
+  EXPECT_TRUE(GetPrivateData()->TrimHistoryIdsPool(&history_id_set));
+  EXPECT_EQ(kItemsToScoreLimit, history_id_set.size());
+
+  for (HistoryID id : history_id_set) {

[dyaroshev, 2017/02/22 22:21:52]

We could do the checks more efficiently for the set but we'll drop the sorting
in the refactoring.

+    if (id < last_high_typed_count)
+      --expected_typed_count;
+    else if (id < last_high_visit_count)
+      --expected_visit_count;
+    else if (id < last_recent)
+      --expected_recent;
+    else if (id < last_worst)
+      --expected_worst;
+  }
+
+  EXPECT_EQ(0, expected_typed_count);
+  EXPECT_EQ(0, expected_visit_count);
+  EXPECT_EQ(0, expected_recent);
+  EXPECT_EQ(0, expected_worst);
+}
+
 TEST_F(InMemoryURLIndexTest, HugeResultSet) {
   // Create a huge set of qualifying history items.
   for (history::URLID row_id = 5000; row_id < 6000; ++row_id) {
     history::URLRow new_row(GURL("http://www.brokeandaloneinmanitoba.com/"),
                             row_id);
     new_row.set_last_visit(base::Time::Now());
     EXPECT_TRUE(UpdateURL(new_row));
   }
 
   ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(
       ASCIIToUTF16("b"), base::string16::npos, kMaxMatches);
-  URLIndexPrivateData& private_data(*GetPrivateData());
   EXPECT_EQ(kMaxMatches, matches.size());
-  // There are 7 matches already in the database.
-  EXPECT_EQ(1008U, private_data.pre_filter_item_count_);
-  EXPECT_EQ(500U, private_data.post_filter_item_count_);
-  EXPECT_EQ(kMaxMatches, private_data.post_scoring_item_count_);
 }
 
 TEST_F(InMemoryURLIndexTest, TitleSearch) {
   // Signal if someone has changed the test DB.
   EXPECT_EQ(30U, GetPrivateData()->history_info_map_.size());
 
   // Ensure title is being searched.
   ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(
       ASCIIToUTF16("MORTGAGE RATE DROPS"), base::string16::npos, kMaxMatches);
   ASSERT_EQ(1U, matches.size());
@@ skipping 605 matching lines @@
   ASSERT_TRUE(GetCacheFilePath(&full_file_path));
   std::vector<base::FilePath::StringType> actual_parts;
   full_file_path.GetComponents(&actual_parts);
   ASSERT_EQ(expected_parts.size(), actual_parts.size());
   size_t count = expected_parts.size();
   for (size_t i = 0; i < count; ++i)
     EXPECT_EQ(expected_parts[i], actual_parts[i]);
   // Must clear the history_dir_ to satisfy the dtor's DCHECK.
   set_history_dir(base::FilePath());
 }