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

[Martin Basti]

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.

2) Please respect PEP8 if it is possible

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...."

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150324/e2626aea/attachment.htm>