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

[thierry bordaz]

On 10/31/2013 11:36 AM, Jan Rusnacko wrote:
> 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 ?
Yes. py.test will record the test as FAILED, call teardown and call the 
next test case
>
> 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

Yes. If a test case does not clean all its temporary components (because 
of uncaught exception or lazy sub-teardown), these components will 
remain. The global teardown will only recreate a new topology. Global 
teardown could be enhanced to do additional cleanup but it will never 
know all temporary created by all test case.
>
> 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.

Yes, a test case may create side effects for the following test cases. 
But I think this risk also exists with one separated file per ticket.

I agree that I need to go with a different approach (one ticket / one file).

regards
thierry

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