<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
On 07/20/2016 05:31 PM, Peter Lacko wrote:<br>
<blockquote
cite="mid:1797731925.7831984.1469028660649.JavaMail.zimbra@redhat.com"
type="cite">
<pre wrap="">Sorry for late reply, I was waiting how the discussion with tracker improvement will end, but since there's no progress and I'm leaving soon, I'm attaching new patch. I also created mapping between old and new tests [1], to make life of reviewer easier. Number in first column denotes line number in original file, followed by line number in new tests file and its name. Tests that are not mentioned in there extend previous coverage.
Regards,
Peter
[1] <a class="moz-txt-link-freetext" href="http://etherpad.corp.redhat.com/Vk3tRLaPSK">http://etherpad.corp.redhat.com/Vk3tRLaPSK</a></pre>
</blockquote>
<br>
<p>Hi,</p>
<p>review notes:</p>
<p>1) code related:</p>
<ul>
<li>leftover PEP8 error:
./ipatests/test_xmlrpc/test_role_plugin.py:696:1: W391 blank
line at end of file</li>
<li>in PrivilegeTracker:</li>
<ul>
<li>creating privilege with description does not work properly,
since there's a hardcoded value in track_create method</li>
<li>the check_retrieve method expects 'description' to be
returned only when privilege is member of a role, but to the
extent of my knowledge that value is returned always</li>
</ul>
<li>in RoleTracker:</li>
<ul>
<li>global variable 'member_types' is used only inside the
class, so it should be a class variable rather than global</li>
<li>'check_add_duplicate_member' method uses oneliner to create
basis of expected value - suggest to use this more in other
methods that do it 'literally' (e.g. check_add_member)</li>
<li>'update_tracker' method - the main else statement should be
investigated more, I'm not sure the try-except part is valid
(if I expect the tracker to delete an attribute, but that
attribute is not present, it's a pass? Doesn't sound right.)</li>
<li>'update' method is needlessly simple - look at the same
method in BaseTracker, which contains more method calls.
Result of the simplicity is that in actual role tests, these
other methods like 'check_update' etc. are called outside the
'update' method, when they can just as well be part of the
method and save repeating same code over and over. This goes
for 'retrieve' and 'find' methods as well.</li>
</ul>
<li>in role tests (ipatests/test_xmlrpc/test_role_plugin.py):</li>
<ul>
<li>in class TestDeleredRole, there's unused method setUpClass</li>
</ul>
</ul>
<p>2) other:</p>
<ul>
<li>I strongly suggest to go through all documentation comments,
since some of them are vague, or even straight misleading (e.g.
in RoleTracker, method 'find_tracker_roles': comment says, that
it performs find command, but nothing like that happens!)</li>
<li>similarly as in previous note, please go through all parameter
descriptions in the documentation - e.g. in RoleTracker, method
'update_tracker': ":param expected_updates: Dictionary of other
attributes" - such description is quite unsatisfactory)</li>
<li>when doing previous two, there are some typos that could be
eliminated</li>
<li>some test cases in ipatests/test_xmlrpc/test_role_plugin.py
seem incorrectly classified, e.g. method 'test_create_invalid'
verifying attempt to create role with invalid name in
TestNonexistentRole class that performes operations like
role-show, role-delete on nonexistent entries</li>
<li>for future patches, it might be nice to have separate patches
for each tracker and the tests<br>
</li>
</ul>
<p>Also, since the author Peter Lacko left the company, fixing this
patch will be a task for Ganna.</p>
<p>Lenka<br>
</p>
<br>
<blockquote
cite="mid:1797731925.7831984.1469028660649.JavaMail.zimbra@redhat.com"
type="cite">
<pre wrap="">
----- Original Message -----
From: "Martin Basti" <a class="moz-txt-link-rfc2396E" href="mailto:mbasti@redhat.com"><mbasti@redhat.com></a>
To: "Peter Lacko" <a class="moz-txt-link-rfc2396E" href="mailto:placko@redhat.com"><placko@redhat.com></a>
Cc: <a class="moz-txt-link-abbreviated" href="mailto:freeipa-devel@redhat.com">freeipa-devel@redhat.com</a>
Sent: Monday, June 6, 2016 12:29:29 PM
Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests
On 02.06.2016 16:16, Peter Lacko wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Rebased with updated tests.
Peter
----- Original Message -----
From: "Martin Basti" <a class="moz-txt-link-rfc2396E" href="mailto:mbasti@redhat.com"><mbasti@redhat.com></a>
To: "Peter Lacko" <a class="moz-txt-link-rfc2396E" href="mailto:placko@redhat.com"><placko@redhat.com></a>
Cc: <a class="moz-txt-link-abbreviated" href="mailto:freeipa-devel@redhat.com">freeipa-devel@redhat.com</a>
Sent: Thursday, June 2, 2016 1:50:06 PM
Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests
Your patch doesn't apply anymore on master, there were changes in role
test and patch have to be updated and rebased
Please look at this for new changes in test_role_plugin.py
<a class="moz-txt-link-freetext" href="https://git.fedorahosted.org/cgit/freeipa.git/commit/?id=5f42b42bd4557a669ab5cfcf1af6596f1a2535f1">https://git.fedorahosted.org/cgit/freeipa.git/commit/?id=5f42b42bd4557a669ab5cfcf1af6596f1a2535f1</a>
Martin^2
On 02.06.2016 11:49, Peter Lacko wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Sorry for late response, I wasn't working these days.
Fixed patch is in attachment.
Peter
----- Original Message -----
From: "Martin Basti" <a class="moz-txt-link-rfc2396E" href="mailto:mbasti@redhat.com"><mbasti@redhat.com></a>
To: "Peter Lacko" <a class="moz-txt-link-rfc2396E" href="mailto:placko@redhat.com"><placko@redhat.com></a>, <a class="moz-txt-link-abbreviated" href="mailto:freeipa-devel@redhat.com">freeipa-devel@redhat.com</a>
Sent: Monday, May 9, 2016 1:06:08 PM
Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests
On 09.05.2016 13:04, Martin Basti wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On 09.05.2016 12:19, Peter Lacko wrote:
</pre>
<blockquote type="cite">
<pre wrap="">+# pylint: disable=unicode-builtin
</pre>
</blockquote>
<pre wrap="">I'm not doing complete review, just the line above hit my eyes.
unicode() is not in Py3 because all strings there are unicode, thus
you cannot use it directly, you need something like
if six.PY2:
str = unicode
and use str() everywhere and remove that #pylint line
FYI all enabled pylint checks are there for a good reason, be careful
with disabling it (mainly disabling it for a whole module) rather ask
before if you are not sure.
Martin^2
</pre>
</blockquote>
<pre wrap="">Nope, sorry, I temporarily forgot how to python
instead of pylint disable use this
if six.PY3:
unicode =str
and keep unicode there. Sorry
Martin^2
</pre>
</blockquote>
</blockquote>
<pre wrap="">Hi,
I don't have time to continue with full review, maybe somebody else can
continue instead of me, but anyway NACK, because using str(unicode()) or
unicode(str()) is bad in both ways, it should be just unicode() in case
of strings (with correct mapping unicode=str in Py3). I just I noticed
this by reading code, but I didn't do deeper review, so there might be
more mistakes.
Martin^2
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<br>
</body>
</html>