[1/2] msi/tests: Test MsiRecordGetString on null and empty strings.

[Nicolas Le Cam]

2009/4/16 James Hawkins <truiken at gmail.com>:
> On Wed, Apr 15, 2009 at 5:21 PM, Nicolas Le Cam <niko.lecam at gmail.com> wrote:
>> 2009/4/16 James Hawkins <truiken at gmail.com>:
>>> On Wed, Apr 15, 2009 at 4:34 PM, Nicolas Le Cam <niko.lecam at gmail.com> wrote:
>>>> While trying to solve ACTION_AppSearchDr problem revealed by my previous
>>>> patch, I discovered that MSI_RecordGetStringW was returning a buffer
>>>> length of 1 on null and empty strings.
>>>>
>>>> Here is the test, the fix follows.
>>>>
>>>> Tested on WinXP and Wine.
>>>>
>>>
>>> This patch has 39 lines of test code without a single empty line.
>>> Unit tests test one thing at a time and should be well documented and
>>> easy to read.
>>>
>>> --
>>> James Hawkins
>>>
>>
>> Hi James,
>>
>> Even if I was tempted to changed it, I tried to follow original code
>> style, as stated multiple times on wine-devel.
>>
>> I will resubmit with empty lines between the different tests.
>>
>
> With a unit test, you're testing one piece of functionality.  Each of
> these tests has a comment above it explaining what you're testing.
>
> /* check behaviour of a record with 0 elements */
>
> The following chunk of code tests different aspects of having a record
> with 0 elements.
>
> You've added to the comment and tests when you really should have
> started a new chunk of tests for the two new cases you're testing.  To
> summarize, you should have three chunks of tests.
> * record with a non-empty string
> * record with null string
> * record with empty string

Test is already testing for null and empty strings but only call
MsiRecordIsNull against them.
It is also doing a lot of test on non empty string in the same
function, even returned buffer length but that was not the purpose of
my patch.

As i'm just completing current test (with a lot of lines I admit)
moving it into it's own chunk seems too much to me.
As I'm only interested in buffer length value, I can limit the test to
that (remove MsiRecordDataSize part and string content verification)
but I thought it was better to have all returned value checked instead
of needing to expand the test another time in case of a regression /
discovery of a new bug.

I will try to make this patch cleaner.

>> What do you mean by well documented ? Isn't the test self explanatory ?
>> I'm setting a null or empty string and verify that getting it back
>> give me an empty string with a null buffer length for both A and W
>> versions.
>>
>
> These tests are rarely self explanatory.  Come back in a couple months
> and try to see what you're testing in those 39 lines.  It won't be
> obvious.
>

It was the first time I looked at this part of wine, and it took me 5
min to understand what the test currently does and what I need to add
to demonstrate the bug I discovered. But it's true that compact code
style didn't help me.

>> Is a comment like "MsiRecordGetString should return an empty string
>> and null buffer length when getting back a null or empty string" will
>> help understanding what I'm trying to do ?
>>
>
> No, the results of the test *are* explanatory.
>
>> BTW, this patch fixes all current failures in wine for msi package
>> tests when run on a root drive dir. Unfortunately it also creates 12
>> new failures. I need to investigate.
>>
>> Seems that this msi patch series will be bigger than expected ;)
>>
>
> I suggest you break the fixes up into small chunks of "fix + test for fix".
>

Doh, I was comparing results with another patch that I'm currently
trying to rework
(http://www.winehq.org/pipermail/wine-patches/2009-April/071756.html).
This series doesn't introduce new failures. It unfortunately doesn't
fix any failures in msi package tests too, (only 12 failures actually
but expected values for 106 tests are wrong if test is run on a root
drive directory). Sorry for the mess.

> --
> James Hawkins
>

Thank you for feedback James, Austin.

-- 
Nicolas Le Cam