Re: bzr commit into mysql-6.0-backup branch (hema:2761) WL#4408

[Chuck Bell]

STATUS
------
Sorry, I must reject the patch.

REQUESTS
--------
1. I understand your argument that compression is not available on all 
Windows builds. However, not testing compression at all on Windows is 
unacceptable. I am aware that there are certain builds of Windows that 
do not include the compression libraries. However, I have it on all of 
my builds (debug or release). We simply must test compression on 
Windows. I cannot accept the patch until we find a way to do so.

Here are some hints:
I see you use the program gunzip. Well, that's most of the problem here. 
You said gzip is not supported on all Windows builds in your explanation 
but that is incorrect. The library zlib is included in debug and release 
Windows builds. However, there are builds being made on Windows that do 
not include the zlib library. It is those builds that we must make the 
test skip, not Windows in general.

I think (but have not been able to confirm) we can detect the absence of 
zlib using the have_compress include file that is already in the test. 
Please ask your fellow QA folks for guidance.

I think you should: a) fix the test to run on Windows, and b) take out 
the steps that unzip the backup image before restore and move them to a 
separate non-windows test if you feel this test case must be included 
and you should include why you feel that test case is important in the 
comments.

2. On that subject, why is backup_errors_compression allowed to run on 
Windows but backup_compression is not? It contradicts your reason for 
not running the compression test on Windows. Of course, if you satisfy 
my request above this point becomes moot.

3. Please document the backup_errors_compression inclusion technique 
(add justification or reason for doing it).

4. While analyzing the backup_compression test I discovered you are 
using a Falcon table but failed to add the include for falcon. Please 
add that to the backup_compression test.

DETAILS
-------
> Thanks for your review comments. Please see my replies inline for your
> questions. I have created new patch satisfying your requests, please
> take a look.
> 
> http://lists.mysql.com/commits/65637
> 
> Thanks,
> Hema.S
> 
> 
> Chuck Bell wrote:
>> STATUS
>> ------
>> Patch rejected. Test failures on Windows.
>>
>> REQUESTS
>> --------
>> 1. Correct platform issue in tests.
>> 2. Is the secure-file-priv really necessary?
>>
>> SUGGESTIONS
>> -----------
>> 3. You should comment every file in the patch headers.
>> 4. Why do we need this?
>>
>> DETAILS
>> -------
>>>  2761 Hema Sridharan    2009-02-06
>>>       WL#4408(Test compression of backup and restore). Made some 
>>> modification in backup_compression.test
>>>       as per review.
>>> added:
>>>   mysql-test/suite/backup/r/backup_errors_compression.result
>>>   mysql-test/suite/backup/t/backup_errors-master.opt
>>>   mysql-test/suite/backup/t/backup_errors_compression-master.opt
>>>   mysql-test/suite/backup/t/backup_errors_compression.test
>>> modified:
>>>   mysql-test/lib/mtr_report.pl
>>>   mysql-test/suite/backup/r/backup_compression.result
>>>   mysql-test/suite/backup/r/backup_errors.result
>>>   mysql-test/suite/backup/t/backup_compression.test
>>>   mysql-test/suite/backup/t/backup_errors.test
>>>
>>> per-file messages:
>>>   mysql-test/lib/mtr_report.pl
>>>     Suppression of warning messages
>>

...

>>> === added file 
>>> 'mysql-test/suite/backup/t/backup_errors_compression-master.opt'
>>> --- 
>>> a/mysql-test/suite/backup/t/backup_errors_compression-master.opt    
>>> 1970-01-01 00:00:00 +0000
>>> +++ 
>>> b/mysql-test/suite/backup/t/backup_errors_compression-master.opt    
>>> 2009-02-06 16:57:20 +0000
>>> @@ -0,0 +1 @@
>>> +--secure-file-priv=
>>
>> [2] Is this really necessary? I don't think it is. If you are setting 
>> it to clear the value, then the problem lies with the test that set it 
>> not this one. Please correctly and/or explain why this has to be done 
>> here this way. Also, FYI see BUG#39581 (in review).
> 
> I have set the "--secure-file-priv= " to test error condition of backup
> to invalid location. I realized that we can test this error condition
> even without specifying "--secure-file-priv= " and therefore, removed
> it. I hope this is fine with you. By doing this test works fine in
> window also.

Ok, I don't like that but seeing no other alternative I accept your 
answer. ;)

>>
>>> === added file 
>>> 'mysql-test/suite/backup/t/backup_errors_compression.test'
>>> --- a/mysql-test/suite/backup/t/backup_errors_compression.test    
>>> 1970-01-01 00:00:00 +0000
>>> +++ b/mysql-test/suite/backup/t/backup_errors_compression.test    
>>> 2009-02-06 16:57:20 +0000
>>> @@ -0,0 +1,9 @@
>>> +#
>>> +# This test is created as part of WL#4408
>>> +# Test all the errors for backup command with compression.
>>> +# Therefore we will extend the backup_errors test for testing error
>>> +# handling of backup with compression.
>>> +#
>>> +
>>> +let $compression=WITH COMPRESSION;
>>
>> [4] Why do we need this? It isn't reset anywhere and only every takes 
>> on the one value. In this case, please do not complicate a test by 
>> adding a variable like this. Please remove the variable.
>>
>>> +--source suite/backup/t/backup_errors.test
> 
> I included the backup_errors_compression.test to test all the error
> conditions in backup using compression. This test replaces $compression
> with "WITH COMPRESSION" in backup_errors.test. I created this test based
> on suggestion from Rafal. Also Rafal sent an email about this proposal
> to dev-backup(on 29th Jan) and I thought everyone is fine with it as
> there was no specific feedback given.

[3] I see. The dirty little 'if none complain then it's ok' rule. 
<aside> My advice is to not use that rule -- some of us will not accept 
an argument based on such. </aside> I looked at the backup_errors test 
and I see the logic. However, this test should make the justification 
and/or decision perfectly clear in the documentation. Please add to the 
comments the reason why you are doing this so people know we aren't 
simply running the same test twice. Note: I don't consider it an 
'extension' of the test -- we can argue the choice of terms but let's 
not go there -- rather, it is simply a change of parameters for the same 
test (nothing is extended).

Chuck