[Freeipa-devel] [PATCH] 0002 New User Role Tests

[Peter Lacko]

Sorry for late reply, I was waiting how the discussion with tracker improvement will end, but since there's no progress and I'm leaving soon, I'm attaching new patch. I also created mapping between old and new tests [1], to make life of reviewer easier. Number in first column denotes line number in original file, followed by line number in new tests file and its name. Tests that are not mentioned in there extend previous coverage.

Regards,
Peter

[1] http://etherpad.corp.redhat.com/Vk3tRLaPSK


----- Original Message -----
From: "Martin Basti" <mbasti at redhat.com>
To: "Peter Lacko" <placko at redhat.com>
Cc: freeipa-devel at redhat.com
Sent: Monday, June 6, 2016 12:29:29 PM
Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests



On 02.06.2016 16:16, Peter Lacko wrote:
> Rebased with updated tests.
>
> Peter
>
> ----- Original Message -----
> From: "Martin Basti" <mbasti at redhat.com>
> To: "Peter Lacko" <placko at redhat.com>
> Cc: freeipa-devel at redhat.com
> Sent: Thursday, June 2, 2016 1:50:06 PM
> Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests
>
> Your patch doesn't apply anymore on master, there were changes in role
> test and patch have to be updated and rebased
>
> Please look at this for new changes in test_role_plugin.py
> https://git.fedorahosted.org/cgit/freeipa.git/commit/?id=5f42b42bd4557a669ab5cfcf1af6596f1a2535f1
>
> Martin^2
>
>
> On 02.06.2016 11:49, Peter Lacko wrote:
>> Sorry for late response, I wasn't working these days.
>> Fixed patch is in attachment.
>>
>> Peter
>>
>>
>> ----- Original Message -----
>> From: "Martin Basti" <mbasti at redhat.com>
>> To: "Peter Lacko" <placko at redhat.com>, freeipa-devel at redhat.com
>> Sent: Monday, May 9, 2016 1:06:08 PM
>> Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests
>>
>>
>>
>> On 09.05.2016 13:04, Martin Basti wrote:
>>> On 09.05.2016 12:19, Peter Lacko wrote:
>>>> +# pylint: disable=unicode-builtin
>>> I'm not doing complete review, just the line above hit my eyes.
>>>
>>> unicode() is not in Py3 because all strings there are unicode, thus
>>> you cannot use it directly, you need something like
>>>
>>> if six.PY2:
>>>       str = unicode
>>>
>>> and use str() everywhere and remove that #pylint line
>>>
>>> FYI all enabled pylint checks are there for a good reason, be careful
>>> with disabling it (mainly disabling it for a whole module) rather ask
>>> before if you are not sure.
>>>
>>> Martin^2
>>>
>> Nope, sorry, I temporarily forgot how to python
>>
>> instead of pylint disable use this
>>
>> if six.PY3:
>>        unicode =str
>>
>> and keep unicode there. Sorry
>>
>> Martin^2
Hi,

I don't have time to continue with full review, maybe somebody else can 
continue instead of me, but anyway NACK, because using str(unicode()) or 
unicode(str()) is bad in both ways, it should be just unicode() in case 
of strings (with correct mapping unicode=str in Py3). I just  I noticed 
this by reading code, but I didn't do deeper review, so there might be 
more mistakes.

Martin^2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-placko-0002-4-New-User-Role-Tests.patch
Type: text/x-patch
Size: 75002 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160720/eb771426/attachment.bin>