<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hi, <div class="moz-cite-prefix">On 03/24/2015 04:40 PM, Martin Basti wrote: </div> <blockquote cite="mid:55118563.2010503@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 24/03/15 14:41, Milan Kubik wrote: </div> <blockquote cite="mid:5511698C.3060305@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> Hello, thanks for the review. <div class="moz-cite-prefix">On 03/24/2015 12:39 PM, Martin Basti wrote: </div> <blockquote cite="mid:55114D0E.8000705@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 17/03/15 10:38, Milan Kubik wrote: </div> <blockquote cite="mid:5507F62E.9080004@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> Hi, <div class="moz-cite-prefix">On 03/16/2015 05:23 PM, Martin Basti wrote: </div> <blockquote cite="mid:55070370.1010200@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 16/03/15 15:32, Milan Kubik wrote: </div> <blockquote cite="mid:5506E983.4020807@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 03/16/2015 12:03 PM, Milan Kubik wrote: </div> <blockquote cite="mid:5506B87B.6050600@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 03/13/2015 02:59 PM, Milan Kubik wrote: </div> <blockquote cite="mid:5502ED38.9020302@redhat.com" type="cite">Hi, this is a patch with port of [1] to pytest. [1]: <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py">https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py</a> Cheers, Milan <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> Added few more asserts in methods where the test could fail and cause other errors. <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> New version of the patch after brief discussion with Martin Basti. Removed unnecessary variable assignments and separated a new test case. <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> 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) </blockquote> done <blockquote cite="mid:55070370.1010200@redhat.com" type="cite"> 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? </blockquote> 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. <blockquote cite="mid:55070370.1010200@redhat.com" type="cite"> Martin^2 </blockquote> Milan <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> 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" + </blockquote> Done. <blockquote cite="mid:55114D0E.8000705@redhat.com" type="cite"> 2) Can you please check if keys were really deleted? + def test_delete_key(self, p11): </blockquote> Done. <blockquote cite="mid:55114D0E.8000705@redhat.com" type="cite"> <pre class="moz-signature" cols="72">-- Martin Basti</pre> </blockquote> 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 </blockquote> 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. </blockquote> fixed (TIL: vim doesn't show the last empty line) <blockquote cite="mid:55118563.2010503@redhat.com" type="cite"> 2) Please respect PEP8 if it is possible </blockquote> Mostly done, there are few instances of long variable names off by few characters. <blockquote cite="mid:55118563.2010503@redhat.com" type="cite"> 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...." </blockquote> Changed to the latter variant. <blockquote cite="mid:55118563.2010503@redhat.com" type="cite"> 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. <pre class="moz-signature" cols="72">-- Martin Basti</pre> </blockquote> Milan </body> </html>