Add unit test for request coordiator

Add unit test for request coordiator

This is an initial set of stub tests for the stub cpp files.
It will allow us to develop tests as we write the code.

BUG=607629
Committed: https://crrev.com/be070e78fb1400dcda73bd03691bfc9182a04e36
Cr-Commit-Position: refs/heads/master@{#391614}

[Pete Williamson, 2016-05-03 22:28:49]

petewil@chromium.org changed reviewers:
+ dougarnett@chromium.org, fgorski@chromium.org

[Pete Williamson, 2016-05-03 22:29:34]

The CQ bit was checked by petewil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-03 22:30:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950733002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950733002/1

[dougarnett, 2016-05-03 22:42:06]

lgtm

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:24: // Queues
|request| to later load and save when system conditions allow.
guess we should comment return value

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:20: 
drop extra line?

[Pete Williamson, 2016-05-03 22:51:47]

CR fixes per DougArnett

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:24: // Queues
|request| to later load and save when system conditions allow.
On 2016/05/03 22:42:06, dougarnett wrote:
> guess we should comment return value

Done.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:20: 
On 2016/05/03 22:42:06, dougarnett wrote:
> drop extra line?

Done.

[fgorski, 2016-05-03 22:55:50]

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:9: #include
"components/offline_pages/background/save_page_request.h"
forward declare instead and add the .h file in .cc

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:18: const
ClientId kClientId("life", "1234");
can we have some test namespace name, that will work later when the policy
arrives?

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:41: bool
callback_called;
callback_called_

Remove please if you don't intend to use it in the test.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:52: void
RequestCoordinatorTest::TearDown() {
not needed.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:55: /* Test
with out/d/bin/run_components_unittests -vv -f RequestCoordinator* */
Don't put that in. Perhaps this go in the test section on the team page.
Also it does not work on my machine as my debug dir is different...

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:57:
TEST_F(RequestCoordinatorTest, StartProcessingTest) {
don't append test to method name. You already have it on the fixture name

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:60:
RequestCoordinator coordinator;
please add a comment above explaining what you are testing for, or rename the
test appropriately, e.g. StartProcessingWithNoRequests

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:61: bool
return_value = coordinator.StartProcessing(callback);
this and next is a single line.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:65:
TEST_F(RequestCoordinatorTest, SavePageLaterTest) {
remove Test from second part.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:68: bool
return_value = coordinator.SavePageLater(request);
single line.

[Pete Williamson, 2016-05-04 17:29:27]

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:9: #include
"components/offline_pages/background/save_page_request.h"
On 2016/05/03 22:55:49, fgorski wrote:
> forward declare instead and add the .h file in .cc

Done.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:18: const
ClientId kClientId("life", "1234");
On 2016/05/03 22:55:49, fgorski wrote:
> can we have some test namespace name, that will work later when the policy
> arrives?

Done.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:41: bool
callback_called;
On 2016/05/03 22:55:50, fgorski wrote:
> callback_called_
> 
> Remove please if you don't intend to use it in the test.

Done.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:52: void
RequestCoordinatorTest::TearDown() {
On 2016/05/03 22:55:50, fgorski wrote:
> not needed.

Done.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:55: /* Test
with out/d/bin/run_components_unittests -vv -f RequestCoordinator* */
On 2016/05/03 22:55:50, fgorski wrote:
> Don't put that in. Perhaps this go in the test section on the team page.
> Also it does not work on my machine as my debug dir is different...

Done.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:57:
TEST_F(RequestCoordinatorTest, StartProcessingTest) {
On 2016/05/03 22:55:50, fgorski wrote:
> don't append test to method name. You already have it on the fixture name

Done.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:60:
RequestCoordinator coordinator;
On 2016/05/03 22:55:50, fgorski wrote:
> please add a comment above explaining what you are testing for, or rename the
> test appropriately, e.g. StartProcessingWithNoRequests

Done.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:61: bool
return_value = coordinator.StartProcessing(callback);
On 2016/05/03 22:55:50, fgorski wrote:
> this and next is a single line.

Done.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:65:
TEST_F(RequestCoordinatorTest, SavePageLaterTest) {
On 2016/05/03 22:55:49, fgorski wrote:
> remove Test from second part.

Done.

https://codereview.chromium.org/1950733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:68: bool
return_value = coordinator.SavePageLater(request);
On 2016/05/03 22:55:50, fgorski wrote:
> single line.

Done.

[fgorski, 2016-05-04 17:39:44]

lgtm with nits and one suggestion: you don't need to support weak ptr here.

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:9: // TODO(fgorski):
include file with SavePageRequest definition.
nit: clean up

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:26: // Returns true if
the page could be queueud successfully.
nit: queued

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:25:
RequestCoordinatorTest();
nit: likely not needed.

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:28: void
callbackFunction() {
nit: use PascalCase not camelCase in C++

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:38:
base::Bind(&RequestCoordinatorTest::callbackFunction, AsWeakPtr());
You can use base::Unretained in tests and not worry about supporting weak ptrs.

[Pete Williamson, 2016-05-04 18:17:42]

More CR feedback per FGorski.

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:9: // TODO(fgorski):
include file with SavePageRequest definition.
On 2016/05/04 17:39:43, fgorski wrote:
> nit: clean up

Done.

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:26: // Returns true if
the page could be queueud successfully.
On 2016/05/04 17:39:44, fgorski wrote:
> nit: queued

Done.

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:25:
RequestCoordinatorTest();
On 2016/05/04 17:39:44, fgorski wrote:
> nit: likely not needed.

I prefer to define these explicitly instead of letting the compiler silently
insert them.

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:28: void
callbackFunction() {
On 2016/05/04 17:39:44, fgorski wrote:
> nit: use PascalCase not camelCase in C++

Done.

https://codereview.chromium.org/1950733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:38:
base::Bind(&RequestCoordinatorTest::callbackFunction, AsWeakPtr());
On 2016/05/04 17:39:44, fgorski wrote:
> You can use base::Unretained in tests and not worry about supporting weak
ptrs.

Done.

[Pete Williamson, 2016-05-04 18:18:02]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-05-04 18:18:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1950733002/#ps60001
(title: "More CR feedback per FGorski")

[commit-bot: I haz the power, 2016-05-04 18:18:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950733002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950733002/60001

[fgorski, 2016-05-04 18:20:17]

one more not needed thing, but you can remove in another CL.

https://codereview.chromium.org/1950733002/diff/60001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/1950733002/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:8: #include
"base/memory/weak_ptr.h"
nit: not needed

[commit-bot: I haz the power, 2016-05-04 20:07:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-04 20:08:28]

Description was changed from

==========
Add unit test for request coordiator

This is an initial set of stub tests for the stub cpp files.
It will allow us to develop tests as we write the code.

BUG=607629
==========

to

==========
Add unit test for request coordiator

This is an initial set of stub tests for the stub cpp files.
It will allow us to develop tests as we write the code.

BUG=607629

Committed: https://crrev.com/be070e78fb1400dcda73bd03691bfc9182a04e36
Cr-Commit-Position: refs/heads/master@{#391614}
==========

[commit-bot: I haz the power, 2016-05-04 20:08:29]

Patchset 4 (id:??) landed as
https://crrev.com/be070e78fb1400dcda73bd03691bfc9182a04e36
Cr-Commit-Position: refs/heads/master@{#391614}