Eliminate the dependency on kernel pseudo-files when running unit tests

Eliminate the dependency on kernel pseudo-files when running unit tests

This change means that the unit tests no longer need /proc/uptime
or /sys/block/sda/stat in order to operate properly.

As an added bonus, this change enables testing that the output of
bootstat_log() exactly matches the expected content.

BUG=chromium-os:8311
TEST=Run the unit tests

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=dda03f9

[kmixter1, 2010-12-06 04:23:52]

LGTM with nits

http://codereview.chromium.org/5500001/diff/3001/Makefile
File Makefile (right):

http://codereview.chromium.org/5500001/diff/3001/Makefile#newcode13
Makefile:13: tests: COMMON_CFLAGS += -DCONFIG_UNIT_TEST
This is a new syntax to me, assuming you know it works and doesn't mess up if
you do make all tests.

http://codereview.chromium.org/5500001/diff/3001/bootstat_log.c
File bootstat_log.c (right):

http://codereview.chromium.org/5500001/diff/3001/bootstat_log.c#newcode63
bootstat_log.c:63: // event_name separately using the "%.*s" format.
Confused by this comment.  Say BOOTSTAT_MAX_EVENT_LEN is 4 and event_name is
abcdefg.  Say sizeof(output_path) == 20.

Result when output_name_prefix="prefix1" would be:
/output/prefix1-abc

Result when output_name_prefix="prefix100000" would be:
/outdir/prefix100000

I guess if is if you assert that something like this holds:

strlen(output_name_prefix) + strlen(output_directory_name) + 2 +
BOOTSTAT_MAX_EVENT_LENGTH <  sizeof(output_path)

http://codereview.chromium.org/5500001/diff/3001/bootstat_log.c#newcode65
bootstat_log.c:65: (void) snprintf(output_path, sizeof(output_path),
"%s/%s-%.*s",
space after (void) seems unusual.  Actually I guess we usually use static_cast
explicitly instead of C-style casts, but even with C-style casts we don't
separate with spaces.

http://codereview.chromium.org/5500001/diff/3001/log_unit_tests.cc
File log_unit_tests.cc (right):

http://codereview.chromium.org/5500001/diff/3001/log_unit_tests.cc#newcode63
log_unit_tests.cc:63: EXPECT_EQ(rv, 0) << file_name << " is not writable: "
You want EXPECT_EQ(0, rv).  constants should always be the first param to get
sensical diagnostics when these fail.

http://codereview.chromium.org/5500001/diff/3001/log_unit_tests.cc#newcode109
log_unit_tests.cc:109: // Bootstat test class.  This class effectively functions
as a mock
i guess this test class is just a test fixture class whosesetup/teardown
routines override boot_log's functionality.  If this were a interface passed
into that boot_log that intercepted its function calls for testing purposes I'd
call it a mock.

http://codereview.chromium.org/5500001/diff/3001/log_unit_tests.cc#newcode189
log_unit_tests.cc:189: << "write to stats file " << uptime_stats_file_name_
indent by 4

[jrbarnette, 2010-12-06 17:47:45]

http://codereview.chromium.org/5500001/diff/3001/Makefile
File Makefile (right):

http://codereview.chromium.org/5500001/diff/3001/Makefile#newcode13
Makefile:13: tests: COMMON_CFLAGS += -DCONFIG_UNIT_TEST
On 2010/12/06 04:23:52, kmixter1 wrote:
> This is a new syntax to me, assuming you know it works and doesn't mess up if
> you do make all tests.

Mmmm...  If you say 'make' or 'make all', or other invocations that build
without -DCONFIG_UNIT_TEST, and then say 'make tests', bootstat.o won't
get rebuilt with the changed define, and the build will fail with link errors.

If you say 'make clean' in between, it works fine.

Is building, then rebuilding with different options (without requiring
'make clean') a use case that should be important?

[kmixter1, 2010-12-06 18:22:36]

FYI

http://codereview.chromium.org/5500001/diff/3001/Makefile
File Makefile (right):

http://codereview.chromium.org/5500001/diff/3001/Makefile#newcode13
Makefile:13: tests: COMMON_CFLAGS += -DCONFIG_UNIT_TEST
On 2010/12/06 17:47:46, jrbarnette wrote:
> On 2010/12/06 04:23:52, kmixter1 wrote:
> > This is a new syntax to me, assuming you know it works and doesn't mess up
if
> > you do make all tests.
> 
> Mmmm...  If you say 'make' or 'make all', or other invocations that build
> without -DCONFIG_UNIT_TEST, and then say 'make tests', bootstat.o won't
> get rebuilt with the changed define, and the build will fail with link errors.
> 
> If you say 'make clean' in between, it works fine.
> 
> Is building, then rebuilding with different options (without requiring
> 'make clean') a use case that should be important?

I was mostly commenting on the fact that the CCFLAGS would be set to having test
flags for even the 'make all' part of the build, but you're correctly pointing
out that the objects themselves are shared and built differently in the two
kinds of builds.

This is your call, but I usually prefer to make the object not have -D's for
test code, just include the test code in production build (particularly if it's
not user visible) as it makes me feel more confidence that there are fewer
differences between test and production code.  It's no more elegant than having
special compilation for tests, but it's easier creating/using a mock interface.

[jrbarnette, 2010-12-06 22:25:00]

New and improved! (details below).  PTAL.


On 2010/12/06 04:23:52, kmixter1 wrote:
> http://codereview.chromium.org/5500001/diff/3001/bootstat_log.c#newcode63
> bootstat_log.c:63: // event_name separately using the "%.*s" format.
> Confused by this comment.  Say BOOTSTAT_MAX_EVENT_LEN is 4 and event_name is
> abcdefg.  Say sizeof(output_path) == 20.
> 
I've changed the comment, hopefully for the better, and added an assert
to ensure that test bugs can't be overlooked.


> Result when output_name_prefix="prefix1" would be:
> /output/prefix1-abc
> 
> Result when output_name_prefix="prefix100000" would be:
> /outdir/prefix100000
> 
> I guess if is if you assert that something like this holds:
> 
> strlen(output_name_prefix) + strlen(output_directory_name) + 2 +
> BOOTSTAT_MAX_EVENT_LENGTH <  sizeof(output_path)
> 
> http://codereview.chromium.org/5500001/diff/3001/bootstat_log.c#newcode65
> bootstat_log.c:65: (void) snprintf(output_path, sizeof(output_path),
> "%s/%s-%.*s",
> space after (void) seems unusual.  Actually I guess we usually use static_cast
> explicitly instead of C-style casts, but even with C-style casts we don't
> separate with spaces.
> 
Within the universe of C coding conventions, it's not so unusual; there's a lot
of
linux kernel code that does this (but not all), and the Solaris kernel standard
makes a specific exception requiring a space..  That said, I'm trying not to
distract the C++ programmers who'll be stuck reading this, so I've dropped
the space.


> http://codereview.chromium.org/5500001/diff/3001/log_unit_tests.cc
> File log_unit_tests.cc (right):
> 
> http://codereview.chromium.org/5500001/diff/3001/log_unit_tests.cc#newcode63
> log_unit_tests.cc:63: EXPECT_EQ(rv, 0) << file_name << " is not writable: "
> You want EXPECT_EQ(0, rv).  constants should always be the first param to get
> sensical diagnostics when these fail.
> 
Ah, quite so.  I see it in the gtest documentation, now that I've
actually looked.  Fixed.

I took the opportunity to refactor the code, because this flaw was
replicated on too many different lines.  :-(


> http://codereview.chromium.org/5500001/diff/3001/log_unit_tests.cc#newcode109
> log_unit_tests.cc:109: // Bootstat test class.  This class effectively
functions
> as a mock
> i guess this test class is just a test fixture class whosesetup/teardown
> routines override boot_log's functionality.  If this were a interface passed
> into that boot_log that intercepted its function calls for testing purposes
I'd
> call it a mock.
> 
I've adjusted the comments.


> http://codereview.chromium.org/5500001/diff/3001/log_unit_tests.cc#newcode189
> log_unit_tests.cc:189: << "write to stats file " << uptime_stats_file_name_
> indent by 4

Fixed.

[jrbarnette, 2010-12-06 22:27:42]

On 2010/12/06 18:22:36, kmixter1 wrote:
[ ... ]
> I was mostly commenting on the fact that the CCFLAGS would be set to having
test
> flags for even the 'make all' part of the build, but you're correctly pointing
> out that the objects themselves are shared and built differently in the two
> kinds of builds.
> 
Oh, yeah, I forgot to answer your question about the funny syntax.  :-(

It's a conditional assignment.  Given this:
  target: FOO = bar
it means "if the make target was 'target', set FOO to 'bar'".


> This is your call, but I usually prefer to make the object not have -D's for
> test code, just include the test code in production build (particularly if
it's
> not user visible) as it makes me feel more confidence that there are fewer
> differences between test and production code.  It's no more elegant than
having
> special compilation for tests, but it's easier creating/using a mock
interface.

Good enough for me; I've nuked the conditional code.

[kmixter1, 2010-12-07 00:30:47]

LGTM