[Freeipa-devel] [PATCH 0002] Refactor test_group_plugin

[Milan Kubík]

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. :)

-- 
Milan Kubik