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

[Jim Meyering]

Petr Uzel wrote:
> On Fri, Aug 21, 2009 at 05:15:34PM +0200, Jim Meyering wrote:
>> Petr Uzel wrote:
>> > * tests/t2300-dos-label-extended-bootcode.sh: New file.
>> > * tests/Makefile.am (TESTS): Add t2300-dos-label-extended-bootcode.sh.
>>
>> Hi Petr,
>>
>> This looks good.
>> A few suggestions below.
>
> Hi Jim,
> Thanks for the comments.
>
>> > +require_512_byte_sector_size_
>> > +
>> > +# Note: the bootcode size is 440B
>>
>> Rather than that comment, please just define a well-named variable,
>>
>> bootcode_size=440
>>
>> And then use it in place of the two hard-coded constants below.
>
> Hmm, actually in the first version of the patch there was this variable, but
> I was convinced not to use it:
> http://www.mail-archive.com/parted-devel@lists.alioth.debian.org/msg02450.html

Don't succumb to requests to unfactor your code ;-)
However, his comment about keeping line length < 80
and judicious variable names is a good one.
If long variable names push too many line lengths beyond 80 (each of which
requires a backslash and continued line, thus *decreasing* readability),
then consider a shorter variable name, but with a comment describing it.
It's always a trade-off.

At least in this case, consistently using a variable for "440"
might have avoided the typo of "400".

>> > +
>> > +test_expect_success \
>> > +  'Install fake bootcode' \
>> > +  'dd if=/dev/urandom of=$dev bs=1c seek=16384 count=400 \
>>
>> Why do you use /dev/urandom?
>> That might not exist.  How about just using e.g., if=Makefile ?
>
> if=Makefile does not work, but if=../Makefile works. BTW the same issue is
> in t0202-gpt-pmbr.sh

Good point.
It's easy to forget that each test is run in a subdirectory.
Considering that, it would be simpler (and slightly more maintainable,
in case the framework ever changes) to use a file that you know exists
in the current directory.  So how about this:

  'printf %0400d 0 > in &&
   dd if=in of=$dev bs=1c seek=16384 count=400 \

>> Shouldn't the above count=400 be count=440 (or now, count=$bootcode_size)?
>
> Yep, looks like a typo.
>
> I'll post a patch to address these issues.

Thanks!