DownloadQuery filters and sorts DownloadItems.

content/browser/download/download_query_unittest.cc

+// Copyright (c) 2011 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 <string>
+
+#include "base/bind.h"
+#include "base/file_path.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "content/browser/download/download_query.h"
+#include "content/browser/download/mock_download_item.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+#include "base/utf_string_conversions.h"
+
+using ::testing::_;
+using ::testing::Return;
+using ::testing::ReturnRef;
+typedef DownloadQuery::DownloadVector DownloadVector;
+
+class DownloadQueryTest : public testing::Test {
+ public:
+  DownloadQueryTest() {}
+  virtual ~DownloadQueryTest() {}
+  virtual void TearDown() {
+    STLDeleteElements(&mocks_);
+  }
+  void CreateMocks(int count) {
+    for (int i = 0; i < count; ++i) {
+      MockDownloadItem* mock = new MockDownloadItem();
+      mocks_.push_back(mock);
+      all_items_.push_back(mock);
+    }
+  }
+  MockDownloadItem& mock(int index) { return *mocks_[index]; }
+  DownloadQuery* query() { return &query_; }
+  DownloadVector* all_items() { return &all_items_; }
+  DownloadVector* results() { return &results_; }
+  void Search() {
+    results_.clear();
+    query()->Search(all_items_.begin(), all_items_.end(), &results_);
+  }
+
+ private:
+  std::vector<MockDownloadItem*> mocks_;
+  DownloadQuery query_;
+  DownloadVector all_items_;

[Randy Smith (Not in Mondays), 2011/12/05 20:25:42]

Why have all_items_ separate from mocks_?  It's not a big deal, but it seems as
if you could just have all_items_ and delete at end from there.

[benjhayden, 2011/12/09 19:29:15]

I didn't want to do that because it would require either instantiating
Search<>() with vector<MockDI*> (which is a different type from vector<DI*>) or
moving Search<>() into the header, but it looks like I can move half of
Search<>() into the header and keep the other half in the cc.
PTAL.

+  DownloadVector results_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadQueryTest);
+};
+
+#define MOCK(_index, _method, _value) \
+  EXPECT_CALL(mock(_index), _method).WillRepeatedly(_value)

[Randy Smith (Not in Mondays), 2011/12/05 20:25:42]

nit, suggestion: My personal reaction to this is that it'd be more readable
without the macro (presuming the audience has familiarity w/ gmock).

[benjhayden, 2011/12/09 19:29:15]

Done.

+
+TEST_F(DownloadQueryTest, EmptyQueryNoItems) {
+  Search();
+  EXPECT_EQ(0U, results()->size());
+}
+
+TEST_F(DownloadQueryTest, EmptyQuerySomeItems) {
+  CreateMocks(3);
+  Search();
+  EXPECT_EQ(3U, results()->size());
+}
+
+TEST_F(DownloadQueryTest, InvalidFilters) {
+  EXPECT_FALSE(query()->Filter(DownloadQuery::FILTER_FIELD_FILENAME_REGEX,
+                               std::string()));
+  EXPECT_FALSE(query()->Filter(DownloadQuery::FILTER_FIELD_URL_REGEX,
+                               string16()));
+  DownloadQuery::FilterFieldName kInvalidName =
+    static_cast<DownloadQuery::FilterFieldName>(kint32max);
+  EXPECT_FALSE(query()->Filter(kInvalidName, 0));
+  EXPECT_FALSE(query()->Filter(kInvalidName, false));
+  EXPECT_FALSE(query()->Filter(kInvalidName, std::string()));
+  EXPECT_FALSE(query()->Filter(kInvalidName, string16()));
+  EXPECT_FALSE(query()->Filter(kInvalidName, DownloadStateInfo::DANGEROUS_URL));
+  EXPECT_FALSE(query()->Filter(kInvalidName, DownloadItem::IN_PROGRESS));
+}
+
+TEST_F(DownloadQueryTest, Limit) {
+  CreateMocks(2);
+  query()->Limit(1);
+  Search();
+  EXPECT_EQ(1U, results()->size());
+}
+
+// Test the GT/LT filters separately to check that I didn't get them backwards.
+TEST_F(DownloadQueryTest, StartedBefore) {
+  CreateMocks(2);
+  MOCK(0, GetStartTime(), Return(base::Time::FromDoubleT(0.001)));

[Randy Smith (Not in Mondays), 2011/12/05 20:25:42]

I get nervous whenever I see code that expects some level of precisions from
doubles.  Would you be willing to do FromInt(1),FromInt(2) and compare against
1000/2000?

[benjhayden, 2011/12/09 19:29:15]

Done.

+  MOCK(1, GetStartTime(), Return(base::Time::FromDoubleT(0.002)));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_STARTED_BEFORE, 2));
+  Search();
+  EXPECT_EQ(1U, results()->size());
+}
+
+TEST_F(DownloadQueryTest, StartedAfter) {
+  CreateMocks(2);
+  MOCK(0, GetStartTime(), Return(base::Time::FromDoubleT(0.001)));
+  MOCK(1, GetStartTime(), Return(base::Time::FromDoubleT(0.002)));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_STARTED_AFTER, 1));
+  Search();
+  EXPECT_EQ(1U, results()->size());
+}
+
+TEST_F(DownloadQueryTest, TotalBytesLess) {
+  CreateMocks(2);
+  MOCK(0, GetTotalBytes(), Return(1));
+  MOCK(1, GetTotalBytes(), Return(2));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_TOTAL_BYTES_LESS, 2));
+  Search();
+  EXPECT_EQ(1U, results()->size());
+}
+
+TEST_F(DownloadQueryTest, TotalBytesGreater) {
+  CreateMocks(2);
+  MOCK(0, GetTotalBytes(), Return(1));
+  MOCK(1, GetTotalBytes(), Return(2));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_TOTAL_BYTES_GREATER, 1));
+  Search();
+  EXPECT_EQ(1U, results()->size());
+}
+
+namespace {
+bool IdNotEqual(int not_id, const DownloadItem& item) {

[Randy Smith (Not in Mondays), 2011/12/05 20:25:42]

nit: I think Chrome custom is for all helper functions for a file to be in a
block at the beginning.

[benjhayden, 2011/12/09 19:29:15]

Done.

+  return item.GetId() != not_id;
+}
+}
+
+TEST_F(DownloadQueryTest, AllFilters) {

[Randy Smith (Not in Mondays), 2011/12/05 20:25:42]

This is a good test for testing all the filters at once, but I'm afraid I also
think you should also have a test that tests each one individually.  If this
test ever rots such that the result wouldn't change if you removed a filter,
that filter could then rot.  Having a separate test per-filter makes that harder
(and also means it's a bit easier to figure out how to add a new thing to filter
on, which is almost certainly going to happen repeatedly).

Same comment applies to sort test below.

[benjhayden, 2011/12/09 19:29:15]

How does this look now with the EXPECT in a loop?

+  // Set up mocks such that only mock(0) matches all filters, and every other
+  // mock fails a different filter (or two for GREATER/LESS filters).
+  CreateMocks(14);
+  FilePath match_filename("match");
+  FilePath fail_filename("fail");
+  GURL match_url("http://example.com/match");
+  GURL fail_url("http://example.com/fail");
+  // Picture a 2D matrix. The rows are MockDownloadItems and the columns are
+  // methods. Every cell contains a value that matches the filters, except for
+  // the diagonal. Every item matches all the filters except one filter, which
+  // it fails, except one item, which matches all the filters without exception.
+  for (size_t i = 0; i < all_items()->size(); ++i) {
+    MOCK(i, GetId(), Return(i));

[Randy Smith (Not in Mondays), 2011/12/05 20:25:42]

Here is a case where I feel some urge to ask you to put in a specialized macro. 
It would take the method to mock, index of that filter, the value to return at
that index, and the type to return at any other index.   I'd also suggest
putting in a constant/define for the number of items, and putting in an
assert/test at the end of the mock list to make it harder to add a new
filter/entry in the list without adding a new mock.

This is a bit of overkill, but I'm trying to make it as easy as possible for
someone who doesn't know the test to add a new filter type to DownloadQuery.

[benjhayden, 2011/12/09 19:29:15]

Done.

+    MOCK(i, GetReceivedBytes(), Return((i == 1) ? 2 : 1));
+    MOCK(i, GetSafetyState(), Return((i == 2) ?
+        DownloadItem::DANGEROUS : DownloadItem::DANGEROUS_BUT_VALIDATED));
+    MOCK(i, GetFullPath(), ReturnRef(
+          (i == 3) ? fail_filename : match_filename));
+    MOCK(i, GetMimeType(), Return((i == 4) ? "image" : "text"));
+    MOCK(i, IsPaused(), Return((i == 5) ? false : true));
+    MOCK(i, MatchesQuery(_), Return((i == 6) ? false : true));
+    MOCK(i, GetStartTime(), Return(base::Time::FromDoubleT(
+        ((i == 7) ? 1.0 : ((i == 8) ? 3.0 : 2.0)) / 1000.0)));
+    MOCK(i, GetTotalBytes(), Return((i == 9) ? 1 : ((i == 10) ? 3 : 2)));
+    MOCK(i, GetOriginalUrl(), ReturnRef((i == 11) ? fail_url : match_url));
+    MOCK(i, GetState(), Return((i == 13) ?
+        DownloadItem::CANCELLED : DownloadItem::IN_PROGRESS));
+    MOCK(i, GetDangerType(), Return((i == 14) ?
+        DownloadStateInfo::DANGEROUS_FILE : DownloadStateInfo::NOT_DANGEROUS));
+  }
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_BYTES_RECEIVED, 1));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_DANGER_ACCEPTED, true));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_FILENAME_REGEX,
+                        string16(ASCIIToUTF16("m"))));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_FILENAME,
+                        string16(ASCIIToUTF16(match_filename.value()))));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_MIME, std::string("text")));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_PAUSED, true));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_QUERY, string16()));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_STARTED_AFTER, 1));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_STARTED_BEFORE, 3));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_START_TIME, 2));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_TOTAL_BYTES_GREATER, 1));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_TOTAL_BYTES_LESS, 3));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_TOTAL_BYTES, 2));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_URL_REGEX,
+                        std::string("m")));
+  CHECK(query()->Filter(DownloadQuery::FILTER_FIELD_URL, match_url.spec()));
+  query()->Filter(base::Bind(&IdNotEqual, 12));
+  query()->Filter(DownloadItem::IN_PROGRESS);
+  query()->Filter(DownloadStateInfo::NOT_DANGEROUS);
+  Search();

[Randy Smith (Not in Mondays), 2011/12/05 20:25:42]

Is it possible to search on a query multiple times?  If so, you could search
after putting in each new filter and confirm that your results dropped by one
each time.  

[benjhayden, 2011/12/09 19:29:15]

Done.

+  EXPECT_EQ(1U, results()->size());
+  EXPECT_EQ(0, results()->at(0)->GetId());
+}
+
+TEST_F(DownloadQueryTest, AllSort) {
+  // mock br d da fn m p st s tb u id dbh

[Randy Smith (Not in Mondays), 2011/12/05 20:25:42]

I think if you're going to have this comment here (and I like it), you should
have a full spelling out of each abbreviation.  Maybe

mock
       Bytes received
            danger type
                Safety state
?

[benjhayden, 2011/12/09 19:29:15]

Done.

+  // 0    1  0 t  a  a t 0  1 8  z 1  12
+  // 1    1  0 t  a  a t 0  1 8  z 1  11
+  // 2    1  0 t  a  a t 0  1 8  z 0  10
+  // 3    1  0 t  a  a t 0  1 8  a 0  9
+  // 4    1  0 t  a  a t 0  1 9  a 0  8
+  // 5    1  0 t  a  a t 0  0 9  a 0  7
+  // 6    1  0 t  a  a t 1  0 9  a 0  6
+  // 7    1  0 t  a  a f 1  0 9  a 0  5
+  // 8    1  0 t  a  z f 1  0 9  a 0  4
+  // 9    1  0 t  z  z f 1  0 9  a 0  3
+  // 10   1  0 f  z  z f 1  0 9  a 0  2
+  // 11   1  1 f  z  z f 1  0 9  a 0  1
+  // 12   0  1 f  z  z f 1  0 9  a 0  0
+  // dbh mock br d da fn m p st s tb u id dbh
+  // 0   12   0  1 f  z  z f 1  0 9  a 0  0
+  // 1   11   1  1 f  z  z f 1  0 9  a 0  1
+  // 2   10   1  0 f  z  z f 1  0 9  a 0  2
+  // 3   9    1  0 t  z  z f 1  0 9  a 0  3
+  // 4   8    1  0 t  a  z f 1  0 9  a 0  4
+  // 5   7    1  0 t  a  a f 1  0 9  a 0  5
+  // 6   6    1  0 t  a  a t 1  0 9  a 0  6
+  // 7   5    1  0 t  a  a t 0  0 9  a 0  7
+  // 8   4    1  0 t  a  a t 0  1 9  a 0  8
+  // 9   3    1  0 t  a  a t 0  1 8  a 0  9
+  // 10  2    1  0 t  a  a t 0  1 8  z 0  10
+  // 11  1    1  0 t  a  a t 0  1 8  z 1  11
+  // 12  0    1  0 t  a  a t 0  1 8  z 1  12
+  CreateMocks(13);
+  FilePath a_filename("a");
+  FilePath z_filename("z");
+  GURL a_url("http://example.com/a");
+  GURL z_url("http://example.com/z");
+  for (size_t i = 0; i < all_items()->size(); ++i) {
+    MOCK(i, GetReceivedBytes(), Return((i >= 12) ? 0 : 1));
+    MOCK(i, GetDangerType(), Return((i >= 11) ?
+        DownloadStateInfo::DANGEROUS_FILE : DownloadStateInfo::NOT_DANGEROUS));
+    MOCK(i, GetSafetyState(), Return((i >= 10) ?
+        DownloadItem::DANGEROUS : DownloadItem::DANGEROUS_BUT_VALIDATED));
+    MOCK(i, GetFullPath(), ReturnRef((i >= 9) ? z_filename : a_filename));
+    MOCK(i, GetMimeType(), Return((i >= 8) ? "z" : "a"));
+    MOCK(i, IsPaused(), Return((i >= 7) ? false : true));
+    MOCK(i, GetStartTime(), Return(base::Time::FromDoubleT(
+        ((i >= 6) ? 2.0 : 1.0) / 1000.0)));
+    MOCK(i, GetState(), Return(
+        (i >= 5) ? DownloadItem::IN_PROGRESS : DownloadItem::COMPLETE));
+    MOCK(i, GetTotalBytes(), Return((i >= 4) ? 1 : 0));
+    MOCK(i, GetOriginalUrl(), ReturnRef((i >= 3) ? a_url : z_url));
+    MOCK(i, GetId(), Return((i >= 2) ? 0 : 1));
+    MOCK(i, GetDbHandle(), Return(all_items()->size() - 1 - i));
+    // all_items() is in exactly the wrong order.
+  }
+  query()->Sort(DownloadQuery::SORT_FIELD_BYTES_RECEIVED,
+                DownloadQuery::ASCENDING);
+  query()->Sort(DownloadQuery::SORT_FIELD_DANGER, DownloadQuery::DESCENDING);
+  query()->Sort(DownloadQuery::SORT_FIELD_DANGER_ACCEPTED,
+                DownloadQuery::ASCENDING);
+  query()->Sort(DownloadQuery::SORT_FIELD_FILENAME, DownloadQuery::DESCENDING);
+  query()->Sort(DownloadQuery::SORT_FIELD_MIME, DownloadQuery::DESCENDING);
+  query()->Sort(DownloadQuery::SORT_FIELD_PAUSED, DownloadQuery::ASCENDING);
+  query()->Sort(DownloadQuery::SORT_FIELD_START_TIME,
+                DownloadQuery::DESCENDING);
+  query()->Sort(DownloadQuery::SORT_FIELD_STATE, DownloadQuery::ASCENDING);
+  query()->Sort(DownloadQuery::SORT_FIELD_TOTAL_BYTES,
+                DownloadQuery::DESCENDING);
+  query()->Sort(DownloadQuery::SORT_FIELD_URL, DownloadQuery::ASCENDING);
+  Search();
+  EXPECT_EQ(13U, results()->size());
+  for (int64 i = 0; i < static_cast<int64>(results()->size()); ++i) {
+    EXPECT_EQ(i, results()->at(i)->GetDbHandle());
+  }
+}