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

[Milan Kubik]

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