[389-devel] Please review lib389 ticket 47578: removal of 'sudo' and absolute path in lib389

[Jan Rusnacko]

On 10/31/2013 11:27 AM, thierry bordaz wrote:
> On 10/30/2013 07:56 PM, Jan Rusnacko wrote:
>> Hello Thierry,
>>
>> layout OK.
>>
>> As for tests - instead of reinventing the wheel by defing class Test_standAlone
>> to set up instance, use py.test fixture.
>>
>> Also, you should not force setup, test, teardown execution for each test by
>> specifying sub-methods for each test. Testing framework (py.test) should be
>> doing that. I think this will make your tests fail badly if some exception
>> occurs - if _test_ticket47560_setup raises exception, it will propagate back to
>> py.test and cleanup method will never be executed for that ticket.
> Hi Jan,
> 
> rereading your comments I realize I misunderstood your point...
> 
> Inside ticket method (e.g. test_ticket47560), the sub-methods (e.g.
> _test_ticket47560_setup or _test_ticket47560_teardown) are not called by py.test
> but by the body of ticket_method. I added them to separate more clearly the
> initialization/cleanup phases of the test case from the real test case. So we
> are not force to define those methods.
Hello Thierry,

I understand this. So if exception occurs anywhere in the testcase (whether in
sub-setup, test of sub-teardown), exception will propagate back to py.test,
which invokes global teardown in the end ?

This works assuming you only want to clean up DS instance. If any ticket would
ever create some temp file and want to remove it after the testcase runs, you
can either
1) put it in the sub-teardown and if the testcase fails, temp file will not be
cleaned up
2) clutter up global teardown method for DS instance, which means global
teardown is now taking care of cleaning up after testcases

Neither is perfect and you might very well end up with testcases with side effects.

I think you are unnecessarily building potential problems doing this.
> 
> The important part is to set the 'clean_please=true' at the beginning of the
> test case, so that if any uncaught exception occurs (and so the test case can
> not cleanup all the initialization it did) the 'teardown' method will remove the
> instance.
> 'clean_please' should be set to 'false' only when the test case completed
> successfully its clean-up.
> 
> regards
> thierry
>>
>> Also, I believe each ticket should have its own file which contains one or more
>> testcases. I think that would reasonably group relevant things together.
>>
>> On 10/30/2013 05:57 PM, thierry bordaz wrote:
>>> https://fedorahosted.org/389/attachment/ticket/47578/0001-Ticket-47578-CI-tests-removal-of-sudo-and-absolute-p.patch
>>>
>>>
>>>
>>>
>>> -- 
>>> 389-devel mailing list
>>> 389-devel at lists.fedoraproject.org
>>> https://admin.fedoraproject.org/mailman/listinfo/389-devel
>>>
>