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

[Milan Kubik]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150324/219f032d/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkubik-0001-5-ipatests-port-of-p11helper-test-from-github.patch
Type: text/x-patch
Size: 12123 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150324/219f032d/attachment.bin>