[parted-devel] [PATCH v2] test for the bootcode-in-extended-partition fix

[Petr Uzel]

Hi Joel & others!

On Tue, May 26, 2009 at 11:38:33AM +0200, Joel Granados wrote:
> Hey Petr.
> 
> My comments bellow.
> On Mon, May 25, 2009 at 01:47:39PM +0200, Petr Uzel wrote:
> > * tests/t2300-dos-label-extended-bootcode.sh: New file.
> > * tests/Makefile.am (TESTS): Add t2300-dos-label-extended-bootcode.sh.
> > 
> > Signed-off-by: Petr Uzel <petr.uzel at suse.cz>
> > ---
> >  tests/Makefile.am                          |    1 +
> >  tests/t2300-dos-label-extended-bootcode.sh |   70 ++++++++++++++++++++++++++++
> >  2 files changed, 71 insertions(+), 0 deletions(-)
> >  create mode 100755 tests/t2300-dos-label-extended-bootcode.sh
<SNIP>
> > +
> > +test_description='Make sure that parted preserves bootcode in extended partition.'
> 
> Having the src code be less than 80 chars in length makes it readable in
> most terminals.  And it looks pretty :).  To this objective I would
> rewrite this to say :
> "
> test_description='Ensure parted preserves bootcode in extended partition.'
> "

ACK

> 
> > +
> > +: ${srcdir=.}
> > +. $srcdir/test-lib.sh
> > +
> > +
> > +dev=loop-file
> > +bootcode_size=440
> > +bootcode_before=before
> > +bootcode_after=after
> 
> I don't see the reason for these variables.  With the "before" and
> "after" variables you are making it longer to write before and after.
> The only reason, to do what you have done (than I can see), is to relate
> them (before and after) to bootcode.  While this is not a bad think, it
> is overkill IMO as the relation is very clear from the code.  If you
> must have this relation explicit, I would use a comment instead of the
> variables.

Those bootcode-* variables (or rather constants) are just a habit from
C, I guess. The 'dev' is taken from some other test.

> 
> For the bootcode_size variable the same applies.  You are needing 13
> chars (bootcode_size) to express 3 (440).  I do understand that not
> everyone knows that the first 440 bytes of a disk is the bootcode, but I
> would handle this with a comment rather than a variable.

ACK, comment is better.

> 
> If you don't use these variables, most of the code bellow fits the 80
> char restriction, which is good :)
> 
> 
> > +
> > +
> > +test_expect_success \
> > +	'Create the test file' \
> > +	'dd if=/dev/zero of=$dev bs=1024c count=100 2>/dev/null'
> 
> We need to redirect the stderr and stdout.  I would rewrite this line as
> follows:
> "
> dd if=/dev/zero of=$dev bs=1024c count=100 > /dev/null 2>&1'
> "

OK

> I would make sure we are redirecting stderr and stdout like that, be it to
> /dev/null or to a file.  This is something that is not done consistently
> in the test, but should be.
> Which reminds me.  I have to correct my tests for this too.
> 
> > +
> > +test_expect_success \
> > +	'Create msdos label' \
> > +	'parted -s $dev mklabel msdos > out 2>&1'
> > +test_expect_success 'Expect no output' 'compare out /dev/null'
> > +
> > +test_expect_success \
> > +	'Create extended partition' \
> > +	'parted -s $dev mkpart extended 32s 127s > out 2>&1'
> > +test_expect_success 'Expect no output' 'compare out /dev/null'
> > +
> > +test_expect_success \
> > +	'Create logical partition' \
> > +	'parted -s $dev mkpart logical 64s 127s > out 2>&1'
> > +test_expect_success 'Expect no output' 'compare out /dev/null'
> > +
> > +test_expect_success \
> > +	'Install fake bootcode' \
> > +	'dd if=/dev/urandom of=$dev bs=1c seek=16384 count=$bootcode_size conv=notrunc 2> /dev/null'
> This is the only line that does not go under 80 chars after changeing
> the variables.
> "
>   'dd if=/dev/urandom of=$dev bs=1c seek=16384 count=440 conv=notrunc > /dev/null 2>&1'
> "
> You can solve this by using the "\" character.  So the line would end up
> looking like this:
> "
>   'dd if=/dev/urandom of=$dev bs=1c seek=16384 count=440 \
>     conv=notrunc > /dev/null 2>&1'
> "

OK

> 
> > +
> > +test_expect_success \
> > +	'Save fake bootcode for later comparison' \
> > +	'dd if=$dev of=$bootcode_before bs=1 skip=16384 count=$bootcode_size 2>/dev/null'
> 
> Here the redirection change.
> 
> > +
> > +test_expect_success \
> > +	'Do something to the label' \
> > +	'parted -s $dev rm 5 > out 2>&1'
> > +test_expect_success 'Expect no output' 'compare out /dev/null'
> > +
> > +test_expect_success \
> > +	'Extract the bootcode for comparison' \
> > +	'dd if=$dev of=$bootcode_after bs=1 skip=16384 count=$bootcode_size 2>/dev/null'
> 
> Here the redirection change.
> 
> > +
> > +test_expect_success \
> > +	'Expect bootcode has not changed' \
> > +	'compare $bootcode_before $bootcode_after'
> 
> Finally, I would not use tabs (in the tests at least).  This is
> something that is not consistent in the tests but should be that way
> because of the GNU coding standards
> (http://www.gnu.org/prep/standards/standards.html#Formatting).  Im
> specifically refering to the example in the body of the function part.
> Consistency in the coding convention is something that parted needs to
> get better at.!!!!!

Thanks for the comments, next version will follow soon.
 

-- 
Best regards / s pozdravem

Petr Uzel, Packages maintainer
---------------------------------------------------------------------
SUSE LINUX, s.r.o.                          e-mail: puzel at suse.cz
Lihovarská 1060/12                          tel: +420 284 028 964
190 00 Prague 9                             fax: +420 284 028 951
Czech Republic                              http://www.suse.cz