Add new testing framework along with a few micro-tests.

Add new testing framework along with a few micro-tests.

This is currently a work-in-progress. However, I have not pestered Louis with
a code review in a while, which means it is time to dump hundreds of lines of
indecipherable shell code in his lap :-)

Included in this patch are several micro-tests:
do_tests.sh: the unit test framework
chip_size.sh: test the ability to print the chip's size
wp_range.sh: test the ability to set the chip's write protect range
wp_toggle.sh: test the ability to turn write protection on or off
partial_writes_x86_bios.sh: test partial writes -- ONLY FOR x86 BIOS, DO NOT USE ON EC
partial_writes_ec.sh: test partial writes for EC (it8500 used for testing, may not work on all ECs)

TODO:
1. The partial_writes tests are pretty basic now. We should do it twice -- once to ensure the blocks actually get written (this may require code changes to flashrom), and once again to ensure regions get skipped appropriately.
2. We should make better use of the SPI emulator.

BUG=
TEST=

[Stefan Reinauer, 2010-11-23 19:05:29]

Awesome!

http://codereview.chromium.org/5136001/diff/3001/util/README
File util/README (right):

http://codereview.chromium.org/5136001/diff/3001/util/README#newcode11
util/README:11: - A version of flashrom which is installed in $PATH
Really? It would be very nice not having to rely on flashrom being installed in
the system path in order to run those tests.
Or is this supposed to be a "known good" version used for recovery at the end?
Please clarify

http://codereview.chromium.org/5136001/diff/3001/util/README#newcode15
util/README:15: FLASHROM_PARAM: Parameters to pass into flashrom
The example below indicates that this would be used to set the programmer to be
used for the testing. Are there limits? Would it make sense to pass -w or -V as
a parameter?

http://codereview.chromium.org/5136001/diff/3001/util/README#newcode17
util/README:17: Important global variables:
Why are you differentiating between global and environment variables?

http://codereview.chromium.org/5136001/diff/3001/util/README#newcode18
util/README:18: BACKUP: The backup copy of ROM image which is read before
executing any tests,
Does this have a default, if BACKUP is not set by the user/tester?

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh
File util/do_tests.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode2
util/do_tests.sh:2: #
With other things living in util/ too and the tests becoming ubiquitous and
countless over times, maybe the tests (and do_tests.sh) should go to their own
tests/ or testing/ directory?

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode5
util/do_tests.sh:5: #
It might also be nice to have a Makefile hook to run the tests. Not sure if this
makes sense with the environment variables required.

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode36
util/do_tests.sh:36: do_test.sh [OPTION] test1.sh test2.sh ... testN.sh
Given that no tests are invoked outside of do_test.sh (is this true?) I think we
could drop the .sh suffix.

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode88
util/do_tests.sh:88: FLASHROM="../flashrom"
It would be nice to be able to run the tests from the top level directory, too.
This could be autodetected, and a variable TESTDIR could point to the tests so
util/do_tests test1 test2 test3 would start the tests from util/ (or testing/ or
whatever)

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_ec.sh
File util/partial_writes_ec.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_ec.sh#ne...
util/partial_writes_ec.sh:149: `printf 0x%x $((${start} + 0x00000))`:`printf
0x%x $((${start} + 0x00fff))` 00_0
Would it make sense to pack this into a separate function?

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_x86_bios.sh
File util/partial_writes_x86_bios.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_x86_bios...
util/partial_writes_x86_bios.sh:77: # Make a layout - 4K regions on 4K
boundaries. This will test basic
Maybe a separate function?

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_x86_bios...
util/partial_writes_x86_bios.sh:155: # Make a layout - 4K regions on 4.5K
boundaries. This will help find problems
ditto

[hailfinger, 2010-11-23 20:05:34]

Looks good for a first attempt.

I'd be happier if the two partial write tests could share more code.

We need to discuss whether running all tests with the default system flashrom
before running them with the to-be-tested flashrom would make the tests more
reliable. And I'm missing cross-checks between the system flashrom and the
to-be-tested flashrom.

If we use the system flashrom for all reads and info retrieval, and the
to-be-tested flashrom for all writes and status changes as well as secondary
verification for the system flashrom reads, we could make sure that no error is
masked.

http://codereview.chromium.org/5136001/diff/3001/util/chip_size.sh
File util/chip_size.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/chip_size.sh#newcode22
util/chip_size.sh:22: if [ ${REPORTED_SIZE} -ne ${ACTUAL_SIZE} ]; then
This only tests that the reported size and the size of the dump file are
consistent. If the wrong size is detected, both the dump and --get-size will
have identical garbage, and the test will pass.

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh
File util/do_tests.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode36
util/do_tests.sh:36: do_test.sh [OPTION] test1.sh test2.sh ... testN.sh
On 2010/11/23 19:05:30, reinauer wrote:
> Given that no tests are invoked outside of do_test.sh (is this true?) I think
we
> could drop the .sh suffix.

The .sh suffix makes it obvious that we're dealing with shell scripts, and you
can actually use
find -name "*.sh"

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode88
util/do_tests.sh:88: FLASHROM="../flashrom"
On 2010/11/23 19:05:30, reinauer wrote:
> It would be nice to be able to run the tests from the top level directory,
too.

Yes, I'll probably start the tests from the top level dir as well. It's just a
lot more convenient.

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode151
util/do_tests.sh:151: flashrom ${FLASHROM_PARAM} -w "$BACKUP"	#FIXME: add this
back in
What should be added back in?

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_ec.sh
File util/partial_writes_ec.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_ec.sh#ne...
util/partial_writes_ec.sh:149: `printf 0x%x $((${start} + 0x00000))`:`printf
0x%x $((${start} + 0x00fff))` 00_0
Can you try to keep the diff to the normal partial write test small? This may
require changes in both scripts, but I believe this is worth it.

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_x86_bios.sh
File util/partial_writes_x86_bios.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_x86_bios...
util/partial_writes_x86_bios.sh:1: #!/bin/sh
Is this script identical to the partial write test already present in mainline
flashrom?

http://codereview.chromium.org/5136001/diff/3001/util/wp-range.sh
File util/wp-range.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/wp-range.sh#newcode45
util/wp-range.sh:45: # next 64K block.
That won't work for most flash chips because each vendor has another protection
scheme, and many only have a all-or-nothing protection.

http://codereview.chromium.org/5136001/diff/3001/util/wp-range.sh#newcode58
util/wp-range.sh:58: tmp=$(./flashrom ${FLASHROM_PARAM} --wp-status 2>/dev/null)
You only check that the printed range and the requested range are identical. If
the range encoding is incorrect, this test will still succeed.

http://codereview.chromium.org/5136001/diff/3001/util/wp-toggle.sh
File util/wp-toggle.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/wp-toggle.sh#newcode57
util/wp-toggle.sh:57: tmp=$(./flashrom ${FLASHROM_PARAM} --wp-status 2>/dev/null
| grep "$MAGIC")
You only check that the output changed, you don't check if the settings on the
chip are correct.

[Stefan Reinauer, 2010-11-23 21:14:14]

http://codereview.chromium.org/5136001/diff/3001/util/chip_size.sh
File util/chip_size.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/chip_size.sh#newcode22
util/chip_size.sh:22: if [ ${REPORTED_SIZE} -ne ${ACTUAL_SIZE} ]; then
Is there anything we can do about that?

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh
File util/do_tests.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode36
util/do_tests.sh:36: do_test.sh [OPTION] test1.sh test2.sh ... testN.sh
Not sure if there is a benefit in "knowing" these are shell scripts unless you
want to change them, in which case you will sure enough find out by opening them
in your editor.
The find does something useful (though not intuitive) until some other utility
ends up in util with a .sh ending. Maybe we should rather call the tests
test_<something>

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_ec.sh
File util/partial_writes_ec.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_ec.sh#ne...
util/partial_writes_ec.sh:149: `printf 0x%x $((${start} + 0x00000))`:`printf
0x%x $((${start} + 0x00fff))` 00_0
Yepp. It would be nice to see some kind of generic pattern and layout generator
that does something useful. Also maybe the printf stuff could be compressed into
some kind of loop

http://codereview.chromium.org/5136001/diff/3001/util/wp-range.sh
File util/wp-range.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/wp-range.sh#newcode45
util/wp-range.sh:45: # next 64K block.
Oh.. would we want some flashrom function to dump blocks and/or protection
blocks?

http://codereview.chromium.org/5136001/diff/3001/util/wp-toggle.sh
File util/wp-toggle.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/wp-toggle.sh#newcode57
util/wp-toggle.sh:57: tmp=$(./flashrom ${FLASHROM_PARAM} --wp-status 2>/dev/null
| grep "$MAGIC")
Wouldn't flashrom's output reflect this? If not, maybe it should?

[dhendrix, 2010-11-24 02:48:14]

Thank you for the review -- You guys were certainly more thorough than I
expected!

Please let me know if there are any lingering concerns for this first attempt.
If not, I'd like to commit it so that we can move on to other pressing matters
such as getting the sources sync'd with upstream. Afterward, we can return to
improve the scripts and make Flashrom more testable.

http://codereview.chromium.org/5136001/diff/3001/util/README
File util/README (right):

http://codereview.chromium.org/5136001/diff/3001/util/README#newcode11
util/README:11: - A version of flashrom which is installed in $PATH
On 2010/11/23 19:05:30, reinauer wrote:
> Really? It would be very nice not having to rely on flashrom being installed
in
> the system path in order to run those tests.
> Or is this supposed to be a "known good" version used for recovery at the end?
> Please clarify

It is intended to be the "known good" version used for obtaining a backup copy
at the beginning and recovery at the end. I've elaborated the comment to reflect
that.

http://codereview.chromium.org/5136001/diff/3001/util/README#newcode15
util/README:15: FLASHROM_PARAM: Parameters to pass into flashrom
On 2010/11/23 19:05:30, reinauer wrote:
> The example below indicates that this would be used to set the programmer to
be
> used for the testing. Are there limits? Would it make sense to pass -w or -V
as
> a parameter?

Good point. Chip operations such as -r and -w are redundant and should
(hopefully) cause flashrom's command parser to fail. -V ought to be fine.

I've added a comment to instruct the user not to specify chip operations.

http://codereview.chromium.org/5136001/diff/3001/util/README#newcode17
util/README:17: Important global variables:
On 2010/11/23 19:05:30, reinauer wrote:
> Why are you differentiating between global and environment variables?

Good question. That bit about global variables does not need to be in this
document. It's intended to let developers know what variables they may use from
the master script.

That sort of thing should be documented in the master script itself, and the
README should be kept simple for normal users.

http://codereview.chromium.org/5136001/diff/3001/util/README#newcode18
util/README:18: BACKUP: The backup copy of ROM image which is read before
executing any tests,
On 2010/11/23 19:05:30, reinauer wrote:
> Does this have a default, if BACKUP is not set by the user/tester?

Heh, this is probably why it should be documented in the code :-)

There is no default -- BACKUP is the image which the known good version of
flashrom reads at the beginning, ie:
BACKUP="backup.bin"
${FLASHROM} ${FLASHROM_PARAM} -r ${BACKUP}

http://codereview.chromium.org/5136001/diff/3001/util/chip_size.sh
File util/chip_size.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/chip_size.sh#newcode22
util/chip_size.sh:22: if [ ${REPORTED_SIZE} -ne ${ACTUAL_SIZE} ]; then
On 2010/11/23 21:14:14, reinauer wrote:
> Is there anything we can do about that?

Hmmm, that's a good point.

Perhaps we also need to compare against the size reported by JEDEC ID (opcode
9fh, ID7-ID0). That will require some further changes in flashrom, so I've added
a TODO.

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh
File util/do_tests.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode2
util/do_tests.sh:2: #
On 2010/11/23 19:05:30, reinauer wrote:
> With other things living in util/ too and the tests becoming ubiquitous and
> countless over times, maybe the tests (and do_tests.sh) should go to their own
> tests/ or testing/ directory?

I tend to agree. It would certainly make it easier to do use wildcards to
specify which tests to copy to my target machines and run :-)

We brought up an idea for organization on the e-mail thread -- Perhaps we can
have a directories for "default" tests (safe, easy to run, maybe a good
candidate for a Makefile hook) and "unsafe" tests which would require special
attention from the user.

That would make it easier to glob a desired subset of tests and help casual
users avoid bricking their system.

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode36
util/do_tests.sh:36: do_test.sh [OPTION] test1.sh test2.sh ... testN.sh
On 2010/11/23 21:14:14, reinauer wrote:
> Not sure if there is a benefit in "knowing" these are shell scripts unless you
> want to change them, in which case you will sure enough find out by opening
them
> in your editor.
> The find does something useful (though not intuitive) until some other utility
> ends up in util with a .sh ending. Maybe we should rather call the tests
> test_<something>

I like the idea of placing the tests in a subdir... that addresses all concerns
and doesn't require a naming convention.

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode88
util/do_tests.sh:88: FLASHROM="../flashrom"
On 2010/11/23 20:05:34, hailfinger wrote:
> On 2010/11/23 19:05:30, reinauer wrote:
> > It would be nice to be able to run the tests from the top level directory,
> too.
> 
> Yes, I'll probably start the tests from the top level dir as well. It's just a
> lot more convenient.

Done.

http://codereview.chromium.org/5136001/diff/3001/util/do_tests.sh#newcode151
util/do_tests.sh:151: flashrom ${FLASHROM_PARAM} -w "$BACKUP"	#FIXME: add this
back in
On 2010/11/23 20:05:34, hailfinger wrote:
> What should be added back in?

stale comment -- removed.

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_ec.sh
File util/partial_writes_ec.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_ec.sh#ne...
util/partial_writes_ec.sh:149: `printf 0x%x $((${start} + 0x00000))`:`printf
0x%x $((${start} + 0x00fff))` 00_0
On 2010/11/23 21:14:14, reinauer wrote:
> Yepp. It would be nice to see some kind of generic pattern and layout
generator
> that does something useful. Also maybe the printf stuff could be compressed
into
> some kind of loop

I managed to compress this stuff into a loop -- good idea!

Unfortunately, I didn't see an obvious way to break the loop into a function and
decrease complexity at the same time. But it definitely looks a lot better now
with the loops and is less prone to stupid copy/paste errors.

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_x86_bios.sh
File util/partial_writes_x86_bios.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_x86_bios...
util/partial_writes_x86_bios.sh:1: #!/bin/sh
On 2010/11/23 20:05:34, hailfinger wrote:
> Is this script identical to the partial write test already present in mainline
> flashrom?

Not quite. The one that is already present is standalone. This one depends on
the master script (do_tests) doing setup/cleanup. Other than that, it's very
similar.

http://codereview.chromium.org/5136001/diff/3001/util/partial_writes_x86_bios...
util/partial_writes_x86_bios.sh:77: # Make a layout - 4K regions on 4K
boundaries. This will test basic
On 2010/11/23 19:05:30, reinauer wrote:
> Maybe a separate function?

As with the EC stuff, I made this into a loop. Making a separate function did
not buy me anything as far as simplifying it.

http://codereview.chromium.org/5136001/diff/3001/util/wp-range.sh
File util/wp-range.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/wp-range.sh#newcode45
util/wp-range.sh:45: # next 64K block.
On 2010/11/23 21:14:14, reinauer wrote:
> Oh.. would we want some flashrom function to dump blocks and/or protection
> blocks?

Correct -- This is sort of a crappy method that will only work for some of the
more recent flash chips with fine-grained block protection.

As Stefan pointed out, the long-term solution is to implement a "--wp-list"
command to list valid write protection values and then pick one that is not
currently in use.

http://codereview.chromium.org/5136001/diff/3001/util/wp-range.sh#newcode58
util/wp-range.sh:58: tmp=$(./flashrom ${FLASHROM_PARAM} --wp-status 2>/dev/null)
On 2010/11/23 20:05:34, hailfinger wrote:
> You only check that the printed range and the requested range are identical.
If
> the range encoding is incorrect, this test will still succeed.

Yeah :-/ For all this test is concerned, one could hard code a range to print
from within flashrom without touching the chip.

I added a #FIXME comment near the top to remind us to revisit this.

http://codereview.chromium.org/5136001/diff/3001/util/wp-toggle.sh
File util/wp-toggle.sh (right):

http://codereview.chromium.org/5136001/diff/3001/util/wp-toggle.sh#newcode57
util/wp-toggle.sh:57: tmp=$(./flashrom ${FLASHROM_PARAM} --wp-status 2>/dev/null
| grep "$MAGIC")
On 2010/11/23 20:05:34, hailfinger wrote:
> You only check that the output changed, you don't check if the settings on the
> chip are correct.

Again, good point. I've added a FIXME near the top of this script as well.

http://codereview.chromium.org/5136001/diff/3001/util/wp-toggle.sh#newcode57
util/wp-toggle.sh:57: tmp=$(./flashrom ${FLASHROM_PARAM} --wp-status 2>/dev/null
| grep "$MAGIC")
On 2010/11/23 21:14:14, reinauer wrote:
> Wouldn't flashrom's output reflect this? If not, maybe it should?

Yes, I should hope that Flashrom does not lie to us. However, Carl-Daniel does
raise a good point about encoders potentially being incorrect.

I think in the long-term, we will have more commands that expose raw
functionality like --spi-rdsr, --spi-rdid, etc. That way we can do a better job
at comparing genuine values from the chip registers rather than comparing print
statements that have run thru potentially erroneous encoders/decoders.