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

--
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