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

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to