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

[James Hawkins]

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

> 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.

> 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".

-- 
James Hawkins