components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc - Issue 2684973014: Only show Last N Pages in the UI when the corresponding tab is visible.

Unified Diff: components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc

Issue 2684973014: Only show Last N Pages in the UI when the corresponding tab is visible. (Closed)

Patch Set: Move impl out Created 3 years, 10 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java ('K') | « components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.cc ('k') | components/offline_pages/core/BUILD.gn » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc

diff --git a/components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc b/components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc

index bc3d6d2f7f00a875bb94981200df1a53a62c2228..0fecc96e67b0164ffcededa90513d7ee04d4e19e 100644

--- a/components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc

+++ b/components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc

@@ -10,14 +10,18 @@

#include "base/bind.h"

#include "base/files/file_path.h"

#include "base/strings/string_number_conversions.h"

+#include "base/test/test_simple_task_runner.h"

+#include "base/threading/thread_task_runner_handle.h"

#include "base/time/time.h"

#include "components/ntp_snippets/category.h"

#include "components/ntp_snippets/content_suggestions_provider.h"

#include "components/ntp_snippets/mock_content_suggestions_provider_observer.h"

#include "components/ntp_snippets/offline_pages/offline_pages_test_utils.h"

+#include "components/offline_pages/core/background/request_coordinator_stub_taco.h"

#include "components/offline_pages/core/client_namespace_constants.h"

+#include "components/offline_pages/core/downloads/download_ui_adapter.h"

#include "components/offline_pages/core/offline_page_item.h"

-#include "components/offline_pages/core/stub_offline_page_model.h"

+#include "components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.h"

#include "components/prefs/testing_pref_service.h"

#include "testing/gmock/include/gmock/gmock.h"

#include "testing/gtest/include/gtest/gtest.h"

@@ -27,7 +31,6 @@ using ntp_snippets::test::FakeOfflinePageModel;

using offline_pages::ClientId;

using offline_pages::MultipleOfflinePageItemCallback;

using offline_pages::OfflinePageItem;

-using offline_pages::StubOfflinePageModel;

using testing::_;

using testing::IsEmpty;

using testing::Mock;

@@ -38,11 +41,14 @@ namespace ntp_snippets {

namespace {

-OfflinePageItem CreateDummyRecentTab(int id) {

- OfflinePageItem item =

- test::CreateDummyOfflinePageItem(id, offline_pages::kLastNNamespace);

- item.client_id.id = base::IntToString(id);

- return item;

+OfflinePageItem CreateDummyRecentTab(int offline_id) {

+ // This is used to assign unique tab IDs to pages. Since offline IDs are

+ // typically small integers like 1, 2, 3 etc, we start at 1001 to ensure that

+ // they are different, and can catch bugs where offline page ID is used in

+ // place of tab ID and vice versa.

+ std::string tab_id = base::IntToString(offline_id + 1000);

+ ClientId client_id(offline_pages::kLastNNamespace, tab_id);

+ return test::CreateDummyOfflinePageItem(offline_id, client_id);

}

std::vector<OfflinePageItem> CreateDummyRecentTabs(

@@ -63,15 +69,25 @@ OfflinePageItem CreateDummyRecentTab(int id, base::Time time) {

} // namespace

-class RecentTabSuggestionsProviderTest : public testing::Test {

+class RecentTabSuggestionsProviderTestNoLoad : public testing::Test {

public:

- RecentTabSuggestionsProviderTest()

- : pref_service_(new TestingPrefServiceSimple()) {

+ RecentTabSuggestionsProviderTestNoLoad()

+ : task_runner_(new base::TestSimpleTaskRunner()),

+ task_runner_handle_(task_runner_),

+ pref_service_(new TestingPrefServiceSimple()) {

RecentTabSuggestionsProvider::RegisterProfilePrefs(

pref_service()->registry());

- provider_.reset(

- new RecentTabSuggestionsProvider(&observer_, &model_, pref_service()));

+ taco_ = base::MakeUnique<offline_pages::RequestCoordinatorStubTaco>();

+ taco_->CreateRequestCoordinator();

+ ui_adapter_ = offline_pages::RecentTabsUIAdapterDelegate::

+ GetOrCreateRecentTabsUIAdapter(&model_, taco_->request_coordinator());

+ delegate_ =

+ offline_pages::RecentTabsUIAdapterDelegate::FromDownloadUIAdapter(

+ ui_adapter_);

+ provider_ = base::MakeUnique<RecentTabSuggestionsProvider>(

+ &observer_, ui_adapter_, pref_service());

}

Category recent_tabs_category() {

@@ -82,18 +98,25 @@ class RecentTabSuggestionsProviderTest : public testing::Test {

return ContentSuggestion::ID(recent_tabs_category(), base::IntToString(id));

}

+ void AddTabAndOfflinePageToModel(const OfflinePageItem& item) {

+ AddTab(offline_pages::RecentTabsUIAdapterDelegate::TabIdFromClientId(

+ item.client_id));

+ AddOfflinePageToModel(item);

+ }

+ void AddTab(int tab_id) { delegate_->RegisterTab(tab_id); }

+ void RemoveTab(int tab_id) { delegate_->UnregisterTab(tab_id); }

void AddOfflinePageToModel(const OfflinePageItem& item) {

- model_.mutable_items()->push_back(item);

- provider_->OfflinePageAdded(&model_, item);

+ ui_adapter_->OfflinePageAdded(&model_, item);

}

void FireOfflinePageDeleted(const OfflinePageItem& item) {

- auto iter = std::remove(model_.mutable_items()->begin(),

- model_.mutable_items()->end(), item);

- auto end = model_.mutable_items()->end();

- model_.mutable_items()->erase(iter, end);

- provider_->OfflinePageDeleted(item.offline_id, item.client_id);

+ int tab_id = offline_pages::RecentTabsUIAdapterDelegate::TabIdFromClientId(

+ item.client_id);

+ RemoveTab(tab_id);

+ ui_adapter_->OfflinePageDeleted(item.offline_id, item.client_id);

}

std::set<std::string> ReadDismissedIDsFromPrefs() {

@@ -101,26 +124,45 @@ class RecentTabSuggestionsProviderTest : public testing::Test {

}

RecentTabSuggestionsProvider* provider() { return provider_.get(); }

- FakeOfflinePageModel* model() { return &model_; }

MockContentSuggestionsProviderObserver* observer() { return &observer_; }

TestingPrefServiceSimple* pref_service() { return pref_service_.get(); }

+ base::TestSimpleTaskRunner* task_runner() { return task_runner_.get(); }

private:

FakeOfflinePageModel model_;

+ offline_pages::DownloadUIAdapter* ui_adapter_;

+ offline_pages::RecentTabsUIAdapterDelegate* delegate_;

+ scoped_refptr<base::TestSimpleTaskRunner> task_runner_;

+ base::ThreadTaskRunnerHandle task_runner_handle_;

+ std::unique_ptr<offline_pages::RequestCoordinatorStubTaco> taco_;

MockContentSuggestionsProviderObserver observer_;

std::unique_ptr<TestingPrefServiceSimple> pref_service_;

// Last so that the dependencies are deleted after the provider.

std::unique_ptr<RecentTabSuggestionsProvider> provider_;

+ DISALLOW_COPY_AND_ASSIGN(RecentTabSuggestionsProviderTestNoLoad);

+};

+// Test that always loads the model before the start of the test.

+class RecentTabSuggestionsProviderTest

+ : public RecentTabSuggestionsProviderTestNoLoad {

+ public:

+ RecentTabSuggestionsProviderTest() = default;

+ void SetUp() override {

+ // The UI adapter always fires asynchronously upon loading, so we want to

+ // run past that moment before each test. Expect a call to hide warnings.

+ EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(1);

+ task_runner()->RunUntilIdle();

+ Mock::VerifyAndClearExpectations(observer());

+ }

+ private:

DISALLOW_COPY_AND_ASSIGN(RecentTabSuggestionsProviderTest);

};

TEST_F(RecentTabSuggestionsProviderTest, ShouldConvertToSuggestions) {

- auto recent_tabs_list = CreateDummyRecentTabs({1, 2});

EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(2);

- for (OfflinePageItem& recent_tab : recent_tabs_list)

- AddOfflinePageToModel(recent_tab);

EXPECT_CALL(

*observer(),

OnNewSuggestions(_, recent_tabs_category(),

@@ -131,7 +173,11 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldConvertToSuggestions) {

GURL("http://dummy.com/2")),

Property(&ContentSuggestion::url,

GURL("http://dummy.com/3")))));

- AddOfflinePageToModel(CreateDummyRecentTab(3));

+ auto recent_tabs_list = CreateDummyRecentTabs({1, 2, 3});

+ for (OfflinePageItem& recent_tab : recent_tabs_list) {

+ AddTabAndOfflinePageToModel(recent_tab);

+ }

}

TEST_F(RecentTabSuggestionsProviderTest, ShouldSortByCreationTime) {

@@ -148,7 +194,7 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldSortByCreationTime) {

OnNewSuggestions(_, recent_tabs_category(),

ElementsAre(Property(&ContentSuggestion::url,

GURL("http://dummy.com/1")))));

- AddOfflinePageToModel(CreateDummyRecentTab(1, now));

+ AddTabAndOfflinePageToModel(CreateDummyRecentTab(1, now));

EXPECT_CALL(

*observer(),

@@ -157,7 +203,7 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldSortByCreationTime) {

ElementsAre(

Property(&ContentSuggestion::url, GURL("http://dummy.com/1")),

Property(&ContentSuggestion::url, GURL("http://dummy.com/2")))));

- AddOfflinePageToModel(CreateDummyRecentTab(2, yesterday));

+ AddTabAndOfflinePageToModel(CreateDummyRecentTab(2, yesterday));

offline_pages[1].last_access_time =

offline_pages[0].last_access_time + base::TimeDelta::FromHours(1);

@@ -172,7 +218,7 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldSortByCreationTime) {

GURL("http://dummy.com/1")),

Property(&ContentSuggestion::url,

GURL("http://dummy.com/2")))));

- AddOfflinePageToModel(CreateDummyRecentTab(3, tomorrow));

+ AddTabAndOfflinePageToModel(CreateDummyRecentTab(3, tomorrow));

}

TEST_F(RecentTabSuggestionsProviderTest, ShouldDeliverCorrectCategoryInfo) {

@@ -189,7 +235,7 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldDismiss) {

EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3);

auto recent_tabs_list = CreateDummyRecentTabs({1, 2, 3});

for (OfflinePageItem& recent_tab : recent_tabs_list) {

- AddOfflinePageToModel(recent_tab);

+ AddTabAndOfflinePageToModel(recent_tab);

}

// Dismiss 2 and 3.

@@ -207,7 +253,7 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldDismiss) {

Property(&ContentSuggestion::url, GURL("http://dummy.com/1")),

Property(&ContentSuggestion::url, GURL("http://dummy.com/4")))));

- AddOfflinePageToModel(CreateDummyRecentTab(4));

+ AddTabAndOfflinePageToModel(CreateDummyRecentTab(4));

Mock::VerifyAndClearExpectations(observer());

// And appear in the dismissed suggestions.

@@ -235,7 +281,7 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldDismiss) {

// And appear in the reported suggestions for the category again.

EXPECT_CALL(*observer(),

OnNewSuggestions(_, recent_tabs_category(), SizeIs(5)));

- AddOfflinePageToModel(CreateDummyRecentTab(5));

+ AddTabAndOfflinePageToModel(CreateDummyRecentTab(5));

Mock::VerifyAndClearExpectations(observer());

}

@@ -244,7 +290,7 @@ TEST_F(RecentTabSuggestionsProviderTest,

EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3);

std::vector<OfflinePageItem> offline_pages = CreateDummyRecentTabs({1, 2, 3});

for (OfflinePageItem& recent_tab : offline_pages)

- AddOfflinePageToModel(recent_tab);

+ AddTabAndOfflinePageToModel(recent_tab);

// Invalidation of suggestion 2 should be forwarded.

EXPECT_CALL(*observer(), OnSuggestionInvalidated(_, GetDummySuggestionId(2)));

@@ -255,7 +301,7 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldClearDismissedOnInvalidate) {

EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3);

std::vector<OfflinePageItem> offline_pages = CreateDummyRecentTabs({1, 2, 3});

for (OfflinePageItem& recent_tab : offline_pages)

- AddOfflinePageToModel(recent_tab);

+ AddTabAndOfflinePageToModel(recent_tab);

EXPECT_THAT(ReadDismissedIDsFromPrefs(), IsEmpty());

provider()->DismissSuggestion(GetDummySuggestionId(2));

@@ -269,7 +315,7 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldClearDismissedOnFetch) {

EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3);

std::vector<OfflinePageItem> offline_pages = CreateDummyRecentTabs({1, 2, 3});

for (OfflinePageItem& recent_tab : offline_pages)

- AddOfflinePageToModel(recent_tab);

+ AddTabAndOfflinePageToModel(recent_tab);

provider()->DismissSuggestion(GetDummySuggestionId(2));

provider()->DismissSuggestion(GetDummySuggestionId(3));

@@ -294,8 +340,8 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldNotShowSameUrlMutlipleTimes) {

// We leave IDs different, but make the URLs the same.

offline_pages[2].url = offline_pages[0].url;

- AddOfflinePageToModel(offline_pages[0]);

- AddOfflinePageToModel(offline_pages[1]);

+ AddTabAndOfflinePageToModel(offline_pages[0]);

+ AddTabAndOfflinePageToModel(offline_pages[1]);

Mock::VerifyAndClearExpectations(observer());

EXPECT_CALL(*observer(),

OnNewSuggestions(

@@ -304,13 +350,11 @@ TEST_F(RecentTabSuggestionsProviderTest, ShouldNotShowSameUrlMutlipleTimes) {

Property(&ContentSuggestion::publish_date, now),

Property(&ContentSuggestion::publish_date, tomorrow))));

- AddOfflinePageToModel(offline_pages[2]);

+ AddTabAndOfflinePageToModel(offline_pages[2]);

}

TEST_F(RecentTabSuggestionsProviderTest,

ShouldNotFetchIfAddedOfflinePageIsNotRecentTab) {

- // The provider is not notified about the first recent tab yet.

- model()->mutable_items()->push_back(CreateDummyRecentTab(1));

// It should not fetch when not a recent tab is added, thus, it should not

// report the first recent tab (which it is not aware about).

EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);

@@ -319,11 +363,27 @@ TEST_F(RecentTabSuggestionsProviderTest,

}

TEST_F(RecentTabSuggestionsProviderTest,

- ShouldFetchIfAddedOfflinePageIsRecentTab) {

- // The provider is not notified about the first recent tab yet.

- model()->mutable_items()->push_back(CreateDummyRecentTab(1));

- // However, it must return the first recent tab (i.e. manually fetch it) even

- // when notified about a different recent tab.

+ ShouldInvalidateSuggestionWhenTabGone) {

+ OfflinePageItem first_tab = CreateDummyRecentTab(1);

+ AddTabAndOfflinePageToModel(first_tab);

+ Mock::VerifyAndClearExpectations(observer());

+ EXPECT_CALL(*observer(), OnSuggestionInvalidated(_, GetDummySuggestionId(1)))

+ .Times(1);

+ RemoveTab(offline_pages::RecentTabsUIAdapterDelegate::TabIdFromClientId(

+ first_tab.client_id));

+ // Removing an unknown tab should not cause extra invalidations.

+ RemoveTab(42);

+TEST_F(RecentTabSuggestionsProviderTest, ShouldNotShowPagesWithoutTab) {

+ EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);

+ // The provider is not notified about the first recent tab yet (no tab).

+ OfflinePageItem first_tab = CreateDummyRecentTab(1);

+ AddOfflinePageToModel(first_tab);

+ Mock::VerifyAndClearExpectations(observer());

+ EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(1);

EXPECT_CALL(

*observer(),

OnNewSuggestions(

@@ -331,7 +391,52 @@ TEST_F(RecentTabSuggestionsProviderTest,

UnorderedElementsAre(

Property(&ContentSuggestion::url, GURL("http://dummy.com/1")),

Property(&ContentSuggestion::url, GURL("http://dummy.com/2")))));

- AddOfflinePageToModel(CreateDummyRecentTab(2));

+ AddTab(offline_pages::RecentTabsUIAdapterDelegate::TabIdFromClientId(

+ first_tab.client_id));

+ OfflinePageItem second_tab = CreateDummyRecentTab(2);

+ AddTabAndOfflinePageToModel(second_tab);

+ Mock::VerifyAndClearExpectations(observer());

+ EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);

+ // |RemoveTab| by itself doesn't cause OnNewSuggestions to be called.

+ RemoveTab(offline_pages::RecentTabsUIAdapterDelegate::TabIdFromClientId(

+ second_tab.client_id));

+ Mock::VerifyAndClearExpectations(observer());

+ // But when we get another tab, OnNewSuggestions will be called.

+ EXPECT_CALL(

+ *observer(),

+ OnNewSuggestions(

+ _, recent_tabs_category(),

+ UnorderedElementsAre(

+ Property(&ContentSuggestion::url, GURL("http://dummy.com/1")),

+ Property(&ContentSuggestion::url, GURL("http://dummy.com/3")))));

+ AddTabAndOfflinePageToModel(CreateDummyRecentTab(3));

+// The following test uses a different fixture that does not automatically pump

+// the event loop in SetUp, which means that until |RunUntilIdle| is called, the

+// UI adapter will not be loaded (and should not fire any events).

+TEST_F(RecentTabSuggestionsProviderTestNoLoad, ShouldFetchOnLoad) {

+ // Tabs are added to the model before the UI adapter is loaded, so there

+ // should only be a single |OnNewSuggestions| call, at load time.

+ EXPECT_CALL(

+ *observer(),

+ OnNewSuggestions(

+ _, recent_tabs_category(),

+ UnorderedElementsAre(

+ Property(&ContentSuggestion::url, GURL("http://dummy.com/1")),

+ Property(&ContentSuggestion::url, GURL("http://dummy.com/2")))));

+ AddTabAndOfflinePageToModel(CreateDummyRecentTab(1));

+ AddTabAndOfflinePageToModel(CreateDummyRecentTab(2));

+ // The provider is not notified about the recent tabs yet.

+ task_runner()->RunUntilIdle();

+ // However, it must return both tabs when the model is loaded.

}

} // namespace ntp_snippets