Skip to content

Commit

Permalink
Add more unit tests for RequestPicker
Browse files Browse the repository at this point in the history 
BUG=610521

Review-Url: https://codereview.chromium.org/2178723002
Cr-Commit-Position: refs/heads/master@{#407901}
  • Loading branch information
petewil authored and Commit bot committed Jul 26, 2016
1 parent 8b69fb4 commit e9e18f8
Show file tree
Hide file tree
Showing 2 changed files with 144 additions and 14 deletions.
25 changes: 21 additions & 4 deletions components/offline_pages/background/offliner_policy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,34 @@ namespace offline_pages {
// RequestCoordinator, some to the RequestQueue, and some to the Offliner.
class OfflinerPolicy {
public:
OfflinerPolicy(){};
OfflinerPolicy()
: prefer_untried_requests_(false),
prefer_earlier_requests_(true),
retry_count_is_more_important_than_recency_(true) {}

OfflinerPolicy(bool prefer_untried,
bool prefer_earlier,
bool prefer_retry_count)
: prefer_untried_requests_(prefer_untried),
prefer_earlier_requests_(prefer_earlier),
retry_count_is_more_important_than_recency_(prefer_retry_count) {}

// TODO(petewil): Numbers here are chosen arbitrarily, do the proper studies
// to get good policy numbers.

// TODO(petewil): Eventually this should get data from a finch experiment.

// Returns true if we should prefer retrying lesser tried requests.
bool ShouldPreferUntriedRequests() { return false; }
bool ShouldPreferUntriedRequests() const { return prefer_untried_requests_; }

// Returns true if we should prefer older requests of equal number of tries.
bool ShouldPreferEarlierRequests() { return true; }
bool ShouldPreferEarlierRequests() const { return prefer_earlier_requests_; }

// Returns true if retry count is considered more important than recency in
// picking which request to try next.
bool RetryCountIsMoreImportantThanRecency() { return true; }
bool RetryCountIsMoreImportantThanRecency() const {
return retry_count_is_more_important_than_recency_;
}

// The max number of times we will retry a request.
int GetMaxRetries() { return kMaxRetries; }
Expand All @@ -54,6 +66,11 @@ class OfflinerPolicy {
int GetMinimumBatteryPercentageForNonUserRequestOfflining() {
return kMinimumBatteryPercentageForNonUserRequestOfflining;
}

private:
bool prefer_untried_requests_;
bool prefer_earlier_requests_;
bool retry_count_is_more_important_than_recency_;
};
}

Expand Down
133 changes: 123 additions & 10 deletions components/offline_pages/background/request_picker_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ const int64_t kRequestId2 = 42;
const GURL kUrl2("http://nytimes.com");
const ClientId kClientId2("bookmark", "5678");
const bool kUserRequested = true;
const int kAttemptCount = 1;

// Constants for policy values - These settings represent the default values.
const bool kPreferUntried = false;
const bool kPreferEarlier = true;
const bool kPreferRetryCount = true;
} // namespace

class RequestPickerTest : public testing::Test {
Expand All @@ -46,6 +52,9 @@ class RequestPickerTest : public testing::Test {

void RequestQueueEmpty();

void QueueRequestsAndChooseOne(const SavePageRequest& request1,
const SavePageRequest& request2);

protected:
// The request queue is simple enough we will use a real queue with a memory
// store instead of a stub.
Expand Down Expand Up @@ -90,14 +99,12 @@ void RequestPickerTest::RequestQueueEmpty() {
request_queue_empty_called_ = true;
}

TEST_F(RequestPickerTest, ChooseNextRequest) {
base::Time creation_time = base::Time::Now();
// Test helper to queue the two given requests and then pick one of them per
// configured policy.
void RequestPickerTest::QueueRequestsAndChooseOne(
const SavePageRequest& request1, const SavePageRequest& request2) {
DeviceConditions conditions;
SavePageRequest request1(
kRequestId1, kUrl1, kClientId1, creation_time, kUserRequested);
SavePageRequest request2(
kRequestId2, kUrl2, kClientId2, creation_time, kUserRequested);
// Put some test requests on the Queue.
// Add test requests on the Queue.
queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,
base::Unretained(this)));
queue_->AddRequest(request2, base::Bind(&RequestPickerTest::AddRequestDone,
Expand All @@ -106,6 +113,7 @@ TEST_F(RequestPickerTest, ChooseNextRequest) {
// Pump the loop to give the async queue the opportunity to do the adds.
PumpLoop();

// Call the method under test.
picker_->ChooseNextRequest(
base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
base::Bind(&RequestPickerTest::RequestQueueEmpty, base::Unretained(this)),
Expand All @@ -115,9 +123,6 @@ TEST_F(RequestPickerTest, ChooseNextRequest) {
// results from the Get operation, and for the picker to call the "picked"
// callback.
PumpLoop();

EXPECT_EQ(kRequestId2, last_picked_->request_id());
EXPECT_FALSE(request_queue_empty_called_);
}

TEST_F(RequestPickerTest, PickFromEmptyQueue) {
Expand All @@ -135,4 +140,112 @@ TEST_F(RequestPickerTest, PickFromEmptyQueue) {
EXPECT_TRUE(request_queue_empty_called_);
}

TEST_F(RequestPickerTest, ChooseRequestWithHigherRetryCount) {
base::Time creation_time = base::Time::Now();
SavePageRequest request1(
kRequestId1, kUrl1, kClientId1, creation_time, kUserRequested);
SavePageRequest request2(
kRequestId2, kUrl2, kClientId2, creation_time, kUserRequested);
request2.set_attempt_count(kAttemptCount);

QueueRequestsAndChooseOne(request1, request2);

EXPECT_EQ(kRequestId2, last_picked_->request_id());
EXPECT_FALSE(request_queue_empty_called_);
}

TEST_F(RequestPickerTest, ChooseRequestWithSameRetryCountButEarlier) {
base::Time creation_time1 =
base::Time::Now() - base::TimeDelta::FromSeconds(10);
base::Time creation_time2 = base::Time::Now();
SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
kUserRequested);
SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
kUserRequested);

QueueRequestsAndChooseOne(request1, request2);

EXPECT_EQ(kRequestId1, last_picked_->request_id());
EXPECT_FALSE(request_queue_empty_called_);
}

TEST_F(RequestPickerTest, ChooseEarlierRequest) {
// We need a custom policy object prefering recency to retry count.
policy_.reset(
new OfflinerPolicy(kPreferUntried, kPreferEarlier, !kPreferRetryCount));
picker_.reset(new RequestPicker(queue_.get(), policy_.get()));

base::Time creation_time1 =
base::Time::Now() - base::TimeDelta::FromSeconds(10);
base::Time creation_time2 = base::Time::Now();
SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
kUserRequested);
SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
kUserRequested);
request2.set_attempt_count(kAttemptCount);

QueueRequestsAndChooseOne(request1, request2);

EXPECT_EQ(kRequestId1, last_picked_->request_id());
EXPECT_FALSE(request_queue_empty_called_);
}

TEST_F(RequestPickerTest, ChooseSameTimeRequestWithHigherRetryCount) {
// We need a custom policy object preferring recency to retry count.
policy_.reset(
new OfflinerPolicy(kPreferUntried, kPreferEarlier, !kPreferRetryCount));
picker_.reset(new RequestPicker(queue_.get(), policy_.get()));

base::Time creation_time = base::Time::Now();
SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
kUserRequested);
SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
kUserRequested);
request2.set_attempt_count(kAttemptCount);

QueueRequestsAndChooseOne(request1, request2);

EXPECT_EQ(kRequestId2, last_picked_->request_id());
EXPECT_FALSE(request_queue_empty_called_);
}

TEST_F(RequestPickerTest, ChooseRequestWithLowerRetryCount) {
// We need a custom policy object preferring lower retry count.
policy_.reset(
new OfflinerPolicy(!kPreferUntried, kPreferEarlier, kPreferRetryCount));
picker_.reset(new RequestPicker(queue_.get(), policy_.get()));

base::Time creation_time = base::Time::Now();
SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
kUserRequested);
SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
kUserRequested);
request2.set_attempt_count(kAttemptCount);

QueueRequestsAndChooseOne(request1, request2);

EXPECT_EQ(kRequestId1, last_picked_->request_id());
EXPECT_FALSE(request_queue_empty_called_);
}

TEST_F(RequestPickerTest, ChooseLaterRequest) {
// We need a custom policy preferring recency over retry, and later requests.
policy_.reset(
new OfflinerPolicy(kPreferUntried, !kPreferEarlier, !kPreferRetryCount));
picker_.reset(new RequestPicker(queue_.get(), policy_.get()));

base::Time creation_time1 =
base::Time::Now() - base::TimeDelta::FromSeconds(10);
base::Time creation_time2 = base::Time::Now();
SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
kUserRequested);
SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
kUserRequested);

QueueRequestsAndChooseOne(request1, request2);

EXPECT_EQ(kRequestId2, last_picked_->request_id());
EXPECT_FALSE(request_queue_empty_called_);
}

} // namespace offline_pages

0 comments on commit e9e18f8

Please sign in to comment.