On Wed, Apr 15, 2009 at 5:21 PM, Nicolas Le Cam <niko.le...@gmail.com> wrote: > 2009/4/16 James Hawkins <trui...@gmail.com>: >> On Wed, Apr 15, 2009 at 4:34 PM, Nicolas Le Cam <niko.le...@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