[flashrom] RFC: Unit testing framework

[David Hendricks]

On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Hi David,
>
> On 23.11.2010 01:31, David Hendricks wrote:
> > Once upon a time, testing flashrom was simple: Read, erase, write,
> verify,
> > done.
> >
> > Over the past few months, we've introduced some very important features
> to
> > Flashrom. Unfortunately, testing is no longer quite so simple. Partial
> > writes require that we test various patterns (and we do with a newly
> > introduced torture script), and we do that to some extend with the
> torture
> > test that we recently checked in. Embedded controllers introduce a whole
> > other world of pain. Long story short, a better testing strategy is
> needed.
> >
>
> Indeed. The biggest problem with any tests right now is that you can
> either write them for the flashrom-chromium tree or for mainline
> flashrom because the flashrom messages are incompatible/different.
>

Ah yes... Flashrom messages have been a pain point for us. The upstream
messages are too verbose, annoy developers, and confuse scripts. The
messages we have now were designed to work around issues as we encountered
them. I hope to work on a long-term fix soon...

On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> > To get the ball rolling, I have a work-in-progress patch for the
> > Chromium.org branch of Flashrom right now to introduce a unit testing
> > framework and would appreciate thoughts / comments:
> > http://codereview.chromium.org/5136001 . You can download the unified
> diff
> > from there (as to avoid duplication here). It's written in shell script
> > (tested on dash and bash) since I felt that's the lowest common
> denominator
> > and the lowest barrier-to-entry for folks wanting to add/modify the
> > tests. Feel free to post comments in this thread, since the code review
> will
> > be closed once the code is checked in.
> >
>
> "Code review will be closed" means the comments will be unaccessible
> after checkin?
>

No. You can still make comments after code is submitted, but new patches
will have a different URL. Either way, I figure it's best to consolidate
general discussion here so that we don't have the discussion spread across
multiple code reviews.

On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> > The idea is simple: One master script does generic setup, backs up the
> > firmware image, runs unit tests, and unconditionally restores the
> firmware
> > image at the end.
>
> Agreed. Here is an interesting question: Given that the initial backup
> and the final restore are done with the system flashrom binary,
> shouldn't we try to restore the backup directly after making the backup?
> If that fails, we can't restore the image at the end, and we're better
> off detecting this problem before we mess with the chip.
>

Good point! However, before we implement that, we'll need to modify flashrom
to force rewrite all blocks. Otherwise, all blocks will simply be skipped.

On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> > The user sets a few (hopefully no more than two or three)
> > environment variables and runs the master script, specifying desired unit
> > tests as arguments on the command line. Each unit test should be
> > independent, and should have its own documentation in case it needs
> special
> > environment variables or something. Due to the variety of tests which may
> be
> > performed, it's up to the user to decide which tests to run on their
> > hardware.
> >
>
> In this case we should warn the user that each of the tests has the
> potential to brick his device. AFAICS the partial write torture test
> can/will result in a hanging EC if the EC code lives in the bottom 128
> kB of the flash chip.
>

An interesting thought, but I think people get used to ignoring warnings.
What if we created two (or more) sub-directories to categorize tests and
keep dangerous ones separate? For starters, let's call them default_tests/
and unsafe_tests/.

default_tests/ would contain tests that any normal user can run relatively
safely to test for simple regressions. We'd put a command on the wiki page
for random interested people to run all of these tests. Maybe even have a
machine @ coreboot.org automatically run them on patches sent to the mailing
list and e-mail a response if it failed.

unsafe_tests/* would have the other crazy shit like EC tests. Users would
need to be much more careful about hardware compatibility, special
environment variables that need to be set, etc.

On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> > If you're curious to try the patch, here is a simple invocation that will
> > test partial writes for an x86 BIOS chip: ./do_tests.sh
> > partial_writes_x86_bios.sh
> >
> > I've attached some sample console output for a test run that does partial
> > writes for x86 BIOS chip and some write protection commands. In this
> > example, I had to pass in some env variables and specify multiple tests
> as
> > arguments. The invocation: FLASHROM="../flashrom" FLASHROM_PARAM="-p
> > internal:bus=spi" sh do_tests.sh partial_writes_x86_bios.sh chip_size.sh
> > wp-toggle.sh wp-range.sh
> >
>
> Why the "sh" after FLASHROM_PARAM="..."?
>

No reason. I often run with sh -x for debugging purposes, that is probably
just a remnant.

On Tue, Nov 23, 2010 at 11:50 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> > If I were to test some EC stuff, I'd need to add additional parameters
> since
> > my EC test script requires a special layout file and an secondary EC
> > firmware image: LAYOUT_FILE=/tmp/ec_layout.txt
> > ALT_EC_IMAGE=/tmp/alt_ec_image.bin FLASHROM="../flashrom"
> >  FLASHROM_PARAM="-p internal:bus=lpc" ./do_tests.sh partial_writes_ec.sh
> >
>
> The big question for the size and protection tests is what you actually
> intend to test and whether the test will work with any chips besides the
> one you wrote the tests for. This is not a complaint directed at the
> RFC, it is rather a hint that writing good tests is hard (but you
> already know that).
>

Yes, indeed! There are numerous assumptions made about the size and
capabilities of the chips that need to be abstracted better. I think your
SPI chip emulator patch will help tremendously here.

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20101123/d366faee/attachment.html>