Re: [1/2] msi/tests: Test MsiRecordGetString on null and empty strings.
2009/4/16 James Hawkins : > On Wed, Apr 15, 2009 at 5:21 PM, Nicolas Le Cam wrote: >> 2009/4/16 James Hawkins : >>> On Wed, Apr 15, 2009 at 4:34 PM, Nicolas Le Cam >>> 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
Re: [1/2] msi/tests: Test MsiRecordGetString on null and empty strings.
On Wed, Apr 15, 2009 at 5:21 PM, Nicolas Le Cam wrote: > 2009/4/16 James Hawkins : >> On Wed, Apr 15, 2009 at 4:34 PM, Nicolas Le Cam 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
Re: [1/2] msi/tests: Test MsiRecordGetString on null and empty strings.
On Wed, Apr 15, 2009 at 7:21 PM, Nicolas Le Cam wrote: > Even if I was tempted to changed it, I tried to follow original code > style, as stated multiple times on wine-devel. While that's encouraged, it's sometimes more of a suggestion than a rule. If you're changing a lot of the code and it helps readability to change the style, go for it. As James said, especially in tests, where each unit test should be it's own standalone piece of code. > 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. > > 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 ? The unit tests should be readable for someone not familiar with Wine, but probably with the Windows API. But comments like: /* Setting a null string */ code... /* Verifying we got a null string back */ > 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. Excellent! Keep up the great work! -- -Austin
Re: [1/2] msi/tests: Test MsiRecordGetString on null and empty strings.
2009/4/16 James Hawkins : > On Wed, Apr 15, 2009 at 4:34 PM, Nicolas Le Cam 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. 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. 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 ? 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 ;) Thanks for the review. -- Nicolas Le Cam
Re: [1/2] msi/tests: Test MsiRecordGetString on null and empty strings.
On Wed, Apr 15, 2009 at 4:34 PM, Nicolas Le Cam 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
