<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 27/03/15 13:52, Milan Kubik wrote:<br>
</div>
<blockquote cite="mid:5515527D.1010805@redhat.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
Hi,<br>
<br>
<div class="moz-cite-prefix">On 03/24/2015 04:40 PM, Martin Basti
wrote:<br>
</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:<br>
</div>
<blockquote cite="mid:5511698C.3060305@redhat.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
Hello,<br>
<br>
thanks for the review.<br>
<br>
<div class="moz-cite-prefix">On 03/24/2015 12:39 PM, Martin
Basti wrote:<br>
</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:<br>
</div>
<blockquote cite="mid:5507F62E.9080004@redhat.com"
type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
Hi,<br>
<br>
<div class="moz-cite-prefix">On 03/16/2015 05:23 PM,
Martin Basti wrote:<br>
</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:<br>
</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:<br>
</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:<br>
</div>
<blockquote cite="mid:5502ED38.9020302@redhat.com"
type="cite">Hi, <br>
<br>
this is a patch with port of [1] to pytest. <br>
<br>
[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>
<br>
<br>
Cheers, <br>
Milan <br>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
Added few more asserts in methods where the test
could fail and cause other errors.<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
New version of the patch after brief discussion with
Martin Basti. Removed unnecessary variable assignments
and separated a new test case.<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
Hello,<br>
<br>
thank you for the patch.<br>
I have a few nitpicks:<br>
1)<br>
You can remove this and use just hexlify(s)<br>
+def str_to_hex(s):<br>
+ return ''.join("{:02x}".format(ord(c)) for c in s)<br>
</blockquote>
done<br>
<blockquote cite="mid:55070370.1010200@redhat.com"
type="cite"> <br>
2)<br>
+ def test_find_secret_key(self, p11):<br>
+ assert
p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY,
label=u"žžž-aest")<br>
<br>
In tests before you tested the exact number of expected
IDs returned by find_keys method, why not here?<br>
</blockquote>
Lack of attention.<br>
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.<br>
<blockquote cite="mid:55070370.1010200@redhat.com"
type="cite"> <br>
Martin^2<br>
</blockquote>
<br>
Milan<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
Thank you for patches, just two nitpicks:<br>
<br>
1)<br>
Can you use the ipaplatform.paths constant? This is platform
specific.<br>
LIBSOFTHSM2_SO = "/usr/lib/pkcs11/libsofthsm2.so"<br>
LIBSOFTHSM2_SO_64 = "/usr/lib64/pkcs11/libsofthsm2.so"<br>
<br>
Respectively use just LIBSOFTHSM2_SO, on 64bit systems it is
automatically mapped into LIBSOFTHSM2_SO_64<br>
<br>
instead of:<br>
+<br>
+libsofthsm = "/usr/lib64/pkcs11/libsofthsm2.so"<br>
+<br>
<br>
</blockquote>
Done.<br>
<blockquote cite="mid:55114D0E.8000705@redhat.com" type="cite">
2)<br>
Can you please check if keys were really deleted?<br>
+ def test_delete_key(self, p11):<br>
</blockquote>
Done.<br>
<blockquote cite="mid:55114D0E.8000705@redhat.com" type="cite">
<pre class="moz-signature" cols="72">--
Martin Basti</pre>
</blockquote>
<br>
I also moved `test_search_for_master_key` right after
`test_generate_master_key` and changed the assert message to a
more specific one.<br>
<br>
Cheers,<br>
Milan<br>
</blockquote>
Please fix this:<br>
<br>
1)<br>
$ git am
freeipa-mkubik-0001-5-ipatests-port-of-p11helper-test-from-github.patch<br>
Applying: ipatests: port of p11helper test from github<br>
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:228: new
blank line at EOF.<br>
+<br>
warning: 1 line adds whitespace errors.<br>
<br>
</blockquote>
fixed (TIL: vim doesn't show the last empty line)<br>
<blockquote cite="mid:55118563.2010503@redhat.com" type="cite"> 2)
Please respect PEP8 if it is possible<br>
</blockquote>
Mostly done, there are few instances of long variable names off by
few characters.<br>
<blockquote cite="mid:55118563.2010503@redhat.com" type="cite"> <br>
3)<br>
I'm still not sure with this:<br>
assert len(master_key) == 0, "The master key should be deleted."<br>
<br>
following example is more pythonic<br>
assert not master_key, "The master key...."<br>
<br>
</blockquote>
Changed to the latter variant.<br>
<blockquote cite="mid:55118563.2010503@redhat.com" type="cite"> 4)<br>
Related to 3), should we test return value, if correct type was
returned? <br>
assert isinstance(master_key, list) and not master_key, "....."<br>
I do not insist on this.<br>
<br>
Otherwise it works as expected.<br>
<pre class="moz-signature" cols="72">--
Martin Basti</pre>
</blockquote>
<br>
Milan<br>
</blockquote>
<br>
Hello,<br>
<br>
I did few modifications:<br>
<br>
* new license header<br>
* PEP8 fixes<br>
* variables instead of magic constants for key labels an IDs<br>
<br>
Patch attached<br>
<br>
Do you accept my modifications?<br>
Martin^2<br>
<pre class="moz-signature" cols="72">--
Martin Basti</pre>
</body>
</html>