[Freeipa-devel] [PATCH 0002] Refactor test_group_plugin

[Milan Kubík]

On 02/10/2016 09:17 AM, Milan Kubík wrote:
> On 02/09/2016 04:19 PM, Milan Kubík wrote:
>> On 01/28/2016 10:42 AM, Filip Skola wrote:
>>>
>>> ----- Original Message -----
>>>> On 01/25/2016 11:11 AM, Filip Skola wrote:
>>>>> ----- Original Message -----
>>>>>> On 01/15/2016 03:38 PM, Filip Skola wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> sending rebased patch.
>>>>>>>
>>>>>>> F.
>>>>>>>
>>>>>>> ----- Original Message -----
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> sorry for delays. The patch no longer applies to master. Rebase 
>>>>>>>> it,
>>>>>>>> please.
>>>>>>>>
>>>>>>>> Milan
>>>>>>>>
>>>>>>>> ----- Original Message -----
>>>>>>>> From: "Filip Škola" <fskola at redhat.com>
>>>>>>>> To: "Milan Kubík" <mkubik at redhat.com>
>>>>>>>> Cc: freeipa-devel at redhat.com
>>>>>>>> Sent: Wednesday, 9 December, 2015 7:01:02 PM
>>>>>>>> Subject: Re: [Freeipa-devel] [PATCH 0002] Refactor 
>>>>>>>> test_group_plugin
>>>>>>>>
>>>>>>>> On Mon, 7 Dec 2015 17:49:18 +0100
>>>>>>>> Milan Kubík <mkubik at redhat.com> wrote:
>>>>>>>>
>>>>>>>>> On 12/03/2015 08:15 PM, Filip Škola wrote:
>>>>>>>>>> On Mon, 30 Nov 2015 17:18:30 +0100
>>>>>>>>>> Milan Kubík <mkubik at redhat.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> On 11/23/2015 04:42 PM, Filip Škola wrote:
>>>>>>>>>>>> Sending updated patch.
>>>>>>>>>>>>
>>>>>>>>>>>> F.
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, 23 Nov 2015 14:59:34 +0100
>>>>>>>>>>>> Filip Škola <fskola at redhat.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Found couple of issues (broke some dependencies).
>>>>>>>>>>>>>
>>>>>>>>>>>>> NACK
>>>>>>>>>>>>>
>>>>>>>>>>>>> F.
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Fri, 20 Nov 2015 13:56:36 +0100
>>>>>>>>>>>>> Filip Škola <fskola at redhat.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Another one.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> F.
>>>>>>>>>>> Hi, the tests look good. Few remarks, though.
>>>>>>>>>>>
>>>>>>>>>>> 1. Please, use the shortes copyright notice in new modules.
>>>>>>>>>>>
>>>>>>>>>>>          #
>>>>>>>>>>>          # Copyright (C) 2015  FreeIPA Contributors see 
>>>>>>>>>>> COPYING for
>>>>>>>>>>> license #
>>>>>>>>>>>
>>>>>>>>>>> 2. The tests `test_group_remove_group_from_protected_group` and
>>>>>>>>>>> `test_group_full_set_of_objectclass_not_available_post_detach`
>>>>>>>>>>> were not ported. Please, include them in the patch.
>>>>>>>>>>>
>>>>>>>>>>> Also, for less hassle, please rebase your patches on top of
>>>>>>>>>>> freeipa-mkubik-0025-3-Separated-Tracker-implementations-into-standalone-pa.patch 
>>>>>>>>>>>
>>>>>>>>>>> Which changes the location of tracker implementations and 
>>>>>>>>>>> prevents
>>>>>>>>>>> circular imports.
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> these cases are there, in corresponding classes. They are marked
>>>>>>>>>> with the original comments. (However I can move them to separate
>>>>>>>>>> class if desirable.)
>>>>>>>>>>
>>>>>>>>>> The copyright notice is changed. Also included a few changes 
>>>>>>>>>> in the
>>>>>>>>>> test with user without private group.
>>>>>>>>>>
>>>>>>>>>> Filip
>>>>>>>>> NACK
>>>>>>>>>
>>>>>>>>> linter:
>>>>>>>>> ************* Module tracker.group_plugin
>>>>>>>>> ipatests/test_xmlrpc/tracker/group_plugin.py:257:
>>>>>>>>> [E0102(function-redefined), GroupTracker.check_remove_member] 
>>>>>>>>> method
>>>>>>>>> already defined line 253)
>>>>>>>>>
>>>>>>>>> Probably a leftover after the rebase made on top of my patch. 
>>>>>>>>> Please
>>>>>>>>> fix it. You can check youch changes by make-lint script before
>>>>>>>>> sending them.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I learned to use make-lint!
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> F.
>>>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> NACK, pylint doesn't seem to like the way the fixtures are imported
>>>>>> (pytest does a lot of runtime magic) [1].
>>>>>> One possible solution would be [2]. Though, I don't think this 
>>>>>> would be
>>>>>> a good idea in our environment. I suggest to create the fixtures 
>>>>>> on per
>>>>>> module basis.
>>>>>>
>>>>>>
>>>>>> [1]: http://fpaste.org/311949/53118942/
>>>>>> [2]:
>>>>>> https://pytest.org/latest/fixture.html#using-fixtures-from-classes-modules-or-projects 
>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> Milan Kubik
>>>>>>
>>>>>>
>>>>> Hi,
>>>>>
>>>>> the fixtures were copied into corresponding module. Please note 
>>>>> that this
>>>>> patch has a dependence on my patch 0001 (user plugin).
>>>>>
>>>>> Filip
>>>> Linter:
>>>> ************* Module ipatests.test_xmlrpc.tracker.group_plugin
>>>> W:100,26: Calling a dict.iter*() method (dict-iter-method)
>>>>
>>>> please use dict.items
>>>>
>>>> -- 
>>>> Milan Kubik
>>>>
>>>>
>>> Hi, sorry. This has been fixed in this patch.
>>>
>>> Filip
>> ACK, thanks for the patience. :)
>>
> Sorry, there are some other things I need clarified. NACK.
> Mail will follow later.
>
What is the purpose of `make_fixture_detach` in your patches? They are 
not used anywhere and the finalizer does nothing.

-- 
Milan Kubik