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

[Milan Kubik]

Hi,

thanks for the review and sparing me few rounds for these modifications. :)

ACK for the improvements.

Milan

On 03/30/2015 10:28 AM, Martin Basti wrote:
> 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/f2e50e05/attachment.htm>