[Freeipa-devel] [PATCH 0001] ipatests: port of p11helper test from github

[Martin Basti]

On 27/03/15 13:52, Milan Kubik wrote:
> Hi,
>
> On 03/24/2015 04:40 PM, Martin Basti wrote:
>> On 24/03/15 14:41, Milan Kubik wrote:
>>> Hello,
>>>
>>> thanks for the review.
>>>
>>> On 03/24/2015 12:39 PM, Martin Basti wrote:
>>>> On 17/03/15 10:38, Milan Kubik wrote:
>>>>> Hi,
>>>>>
>>>>> On 03/16/2015 05:23 PM, Martin Basti wrote:
>>>>>> On 16/03/15 15:32, Milan Kubik wrote:
>>>>>>> On 03/16/2015 12:03 PM, Milan Kubik wrote:
>>>>>>>> On 03/13/2015 02:59 PM, Milan Kubik wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> this is a patch with port of [1] to pytest.
>>>>>>>>>
>>>>>>>>> [1]: 
>>>>>>>>> https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Milan
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>> Added few more asserts in methods where the test could fail and 
>>>>>>>> cause other errors.
>>>>>>>>
>>>>>>>>
>>>>>>> New version of the patch after brief discussion with Martin 
>>>>>>> Basti. Removed unnecessary variable assignments and separated a 
>>>>>>> new test case.
>>>>>>>
>>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> thank you for the patch.
>>>>>> I have a few nitpicks:
>>>>>> 1)
>>>>>> You can remove this and use just hexlify(s)
>>>>>> +def str_to_hex(s):
>>>>>> +    return ''.join("{:02x}".format(ord(c)) for c in s)
>>>>> done
>>>>>>
>>>>>> 2)
>>>>>> + def test_find_secret_key(self, p11):
>>>>>> +     assert p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY, 
>>>>>> label=u"žžž-aest")
>>>>>>
>>>>>> In tests before you tested the exact number of expected IDs 
>>>>>> returned by find_keys method, why not here?
>>>>> Lack of attention.
>>>>> Fixed the assert in `test_search_for_master_key` which does the 
>>>>> same thing. Merged `test_find_secret_key` with 
>>>>> `test_search_for_master_key` where it belongs.
>>>>>>
>>>>>> Martin^2
>>>>>
>>>>> Milan
>>>>>
>>>>>
>>>> Thank you for patches, just two nitpicks:
>>>>
>>>> 1)
>>>> Can you use the ipaplatform.paths constant? This is platform specific.
>>>> LIBSOFTHSM2_SO = "/usr/lib/pkcs11/libsofthsm2.so"
>>>> LIBSOFTHSM2_SO_64 = "/usr/lib64/pkcs11/libsofthsm2.so"
>>>>
>>>> Respectively use just LIBSOFTHSM2_SO, on 64bit systems it is 
>>>> automatically mapped into LIBSOFTHSM2_SO_64
>>>>
>>>> instead of:
>>>> +
>>>> +libsofthsm = "/usr/lib64/pkcs11/libsofthsm2.so"
>>>> +
>>>>
>>> Done.
>>>> 2)
>>>> Can you please check if keys were really deleted?
>>>> +    def test_delete_key(self, p11):
>>> Done.
>>>> -- 
>>>> Martin Basti
>>>
>>> I also moved `test_search_for_master_key` right after 
>>> `test_generate_master_key` and changed the assert message to a more 
>>> specific one.
>>>
>>> Cheers,
>>> Milan
>> Please fix this:
>>
>> 1)
>> $ git am 
>> freeipa-mkubik-0001-5-ipatests-port-of-p11helper-test-from-github.patch
>> Applying: ipatests: port of p11helper test from github
>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:228: new 
>> blank line at EOF.
>> +
>> warning: 1 line adds whitespace errors.
>>
> fixed (TIL: vim doesn't show the last empty line)
>> 2) Please respect PEP8 if it is possible
> Mostly done, there are few instances of long variable names off by few 
> characters.
>>
>> 3)
>> I'm still not sure with this:
>> assert len(master_key) == 0, "The master key should be deleted."
>>
>> following example is more pythonic
>> assert not master_key, "The master key...."
>>
> Changed to the latter variant.
>> 4)
>> Related to 3), should we test return value, if correct type was 
>> returned?
>> assert isinstance(master_key, list) and not master_key, "....."
>> I do not insist on this.
>>
>> Otherwise it works as expected.
>> -- 
>> Martin Basti
>
> Milan

Hello,

I did few modifications:

* new license header
* PEP8 fixes
* variables instead of magic constants for key labels an IDs

Patch attached

Do you accept my modifications?
Martin^2

-- 
Martin Basti

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150330/b70d4d3a/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkubik+mbasti-0001.7-ipatests-port-of-p11helper-test-from-github.patch
Type: text/x-patch
Size: 14288 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150330/b70d4d3a/attachment.bin>