[Freeipa-devel] [PATCH 0002] Refactor test_group_plugin

[Milan Kubík]

On 02/22/2016 11:09 AM, Filip Skola wrote:
>
> ----- Original Message -----
>> 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
>>
>>
> Hi,
>
> none, I guess, probably a leftover copied from the tracker in the early days. Deleting the function.
>
> Filip
Ack.

-- 
Milan Kubik