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

[Jan Cholasta]

Hi,

we reply below original message on freeipa-devel, please follow this 
convention next time, thank you.

Dne 30.3.2015 v 13:33 Milan Kubik napsal(a):
> 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

Pushed to master: 59f024487e0bcaedb773fd4066b2f95c733278c6

-- 
Jan Cholasta