Add some basic tests for metrics_daemon.

Add some basic tests for metrics_daemon.

A separate CL adds a test stanza to the metrics ebuild. More tests
(specifically, for the D-Bus MessageFilter) will also come in a
separate CL.

[sosa, 2010-05-05 21:46:56]

http://codereview.chromium.org/1919005/diff/1/2
File src/platform/metrics/Makefile (right):

http://codereview.chromium.org/1919005/diff/1/2#newcode15
src/platform/metrics/Makefile:15: DAEMON_TEST = metrics_deamon_test
Misspelling of daemon?

http://codereview.chromium.org/1919005/diff/1/7
File src/platform/metrics/metrics_daemon_test.cc (right):

http://codereview.chromium.org/1919005/diff/1/7#newcode15
src/platform/metrics/metrics_daemon_test.cc:15: static const char
kDailyUseRecordFile[] = "/tmp/daily-usage-test";
if you do import, prob rename this to kDailyTestUseRecordFile or the like.  it's
already a tiny bit confusing having the same constant in two different classes
have two different definitions.

http://codereview.chromium.org/1919005/diff/1/7#newcode27
src/platform/metrics/metrics_daemon_test.cc:27: static const int kSecondsPerDay
= 24 * 60 * 60;
can't you just declare the const in metrics_daemon.h and get from
metrics_daemon?

http://codereview.chromium.org/1919005/diff/1/7#newcode137
src/platform/metrics/metrics_daemon_test.cc:137: EXPECT_EQ(0,
daemon_.daily_use_day_last_);
Please use NULL instead of 0

[petkov, 2010-05-05 23:03:34]

Thanks for the quick review. PTAL.

http://codereview.chromium.org/1919005/diff/1/2
File src/platform/metrics/Makefile (right):

http://codereview.chromium.org/1919005/diff/1/2#newcode15
src/platform/metrics/Makefile:15: DAEMON_TEST = metrics_deamon_test
On 2010/05/05 21:46:56, sosa wrote:
> Misspelling of daemon?

Done.

http://codereview.chromium.org/1919005/diff/1/7
File src/platform/metrics/metrics_daemon_test.cc (right):

http://codereview.chromium.org/1919005/diff/1/7#newcode15
src/platform/metrics/metrics_daemon_test.cc:15: static const char
kDailyUseRecordFile[] = "/tmp/daily-usage-test";
On 2010/05/05 21:46:56, sosa wrote:
> if you do import, prob rename this to kDailyTestUseRecordFile or the like. 
it's
> already a tiny bit confusing having the same constant in two different classes
> have two different definitions.

Done.

http://codereview.chromium.org/1919005/diff/1/7#newcode27
src/platform/metrics/metrics_daemon_test.cc:27: static const int kSecondsPerDay
= 24 * 60 * 60;
On 2010/05/05 21:46:56, sosa wrote:
> can't you just declare the const in metrics_daemon.h and get from
> metrics_daemon?
As discussed in person, there're a number of issues associated with this. But
moved most of the other constants.

http://codereview.chromium.org/1919005/diff/1/7#newcode137
src/platform/metrics/metrics_daemon_test.cc:137: EXPECT_EQ(0,
daemon_.daily_use_day_last_);
On 2010/05/05 21:46:56, sosa wrote:
> Please use NULL instead of 0

daily_use_day_last_ is int.

[sosa, 2010-05-05 23:04:11]

lgtm

On Wed, May 5, 2010 at 4:03 PM,  <petkov@chromium.org> wrote:
> Thanks for the quick review. PTAL.
>
>
>
>
> http://codereview.chromium.org/1919005/diff/1/2
> File src/platform/metrics/Makefile (right):
>
> http://codereview.chromium.org/1919005/diff/1/2#newcode15
> src/platform/metrics/Makefile:15: DAEMON_TEST = metrics_deamon_test
> On 2010/05/05 21:46:56, sosa wrote:
>>
>> Misspelling of daemon?
>
> Done.
>
> http://codereview.chromium.org/1919005/diff/1/7
> File src/platform/metrics/metrics_daemon_test.cc (right):
>
> http://codereview.chromium.org/1919005/diff/1/7#newcode15
> src/platform/metrics/metrics_daemon_test.cc:15: static const char
> kDailyUseRecordFile[] = "/tmp/daily-usage-test";
> On 2010/05/05 21:46:56, sosa wrote:
>>
>> if you do import, prob rename this to kDailyTestUseRecordFile or the
>
> like.  it's
>>
>> already a tiny bit confusing having the same constant in two different
>
> classes
>>
>> have two different definitions.
>
> Done.
>
> http://codereview.chromium.org/1919005/diff/1/7#newcode27
> src/platform/metrics/metrics_daemon_test.cc:27: static const int
> kSecondsPerDay = 24 * 60 * 60;
> On 2010/05/05 21:46:56, sosa wrote:
>>
>> can't you just declare the const in metrics_daemon.h and get from
>> metrics_daemon?
>
> As discussed in person, there're a number of issues associated with
> this. But moved most of the other constants.
>
> http://codereview.chromium.org/1919005/diff/1/7#newcode137
> src/platform/metrics/metrics_daemon_test.cc:137: EXPECT_EQ(0,
> daemon_.daily_use_day_last_);
> On 2010/05/05 21:46:56, sosa wrote:
>>
>> Please use NULL instead of 0
>
> daily_use_day_last_ is int.
>
> http://codereview.chromium.org/1919005/show
>

[kmixter1, 2010-05-10 19:06:30]

FYI

Nice!

http://codereview.chromium.org/1919005/diff/5001/6003
File src/platform/metrics/metrics_daemon.cc (right):

http://codereview.chromium.org/1919005/diff/5001/6003#newcode401
src/platform/metrics/metrics_daemon.cc:401: if (testing_)
Ideally you'd want to make this function available through a fake/mock object.

http://codereview.chromium.org/1919005/diff/5001/6006
File src/platform/metrics/metrics_daemon_test.cc (right):

http://codereview.chromium.org/1919005/diff/5001/6006#newcode126
src/platform/metrics/metrics_daemon_test.cc:126: bool NoOrEmptyUseRecordFile() {
Function names should start with verb

[petkov, 2010-05-10 22:28:02]

Thanks for looking at this.

On 2010/05/10 19:06:30, kmixter1 wrote:
> FYI
> 
> Nice!
> 
> http://codereview.chromium.org/1919005/diff/5001/6003
> File src/platform/metrics/metrics_daemon.cc (right):
> 
> http://codereview.chromium.org/1919005/diff/5001/6003#newcode401
> src/platform/metrics/metrics_daemon.cc:401: if (testing_)
> Ideally you'd want to make this function available through a fake/mock object.

Yes, I kind of followed the existing code. I'm also considering mocking time,
glib, dbus to make the unit test more complete...

> 
> http://codereview.chromium.org/1919005/diff/5001/6006
> File src/platform/metrics/metrics_daemon_test.cc (right):
> 
> http://codereview.chromium.org/1919005/diff/5001/6006#newcode126
> src/platform/metrics/metrics_daemon_test.cc:126: bool NoOrEmptyUseRecordFile()
{
> Function names should start with verb

Good to know, I'll fix. The style guide kind of mentions it in one place but
then gives "MyExcitingMethod()" as an example.