<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Gabe,<br>
<br>
sorry for the delay. Here comes the review!<br>
<br>
1.) All the tests fail, since the IPA master is not installed at
all:<br>
<br>
<pre id="out" class="console-output"><pre> def test_advice(self):
# Obtain the advice from the server
> tasks.kinit_admin(self.master)
test_integration/test_advise.py:37:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test_integration/tasks.py:484: in kinit_admin
stdin_text=host.config.admin_password)
../pytest_multihost/host.py:222: in run_command
command.wait(raiseonerr=raiseonerr)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <pytest_multihost.transport.SSHCommand object at 0x7f09c0530c90>
raiseonerr = True
def wait(self, raiseonerr=True):
"""Wait for the remote process to exit
Raises an excption if the exit code is not 0, unless raiseonerr is
true.
"""
if self._done:
return self.returncode
self._end_process()
self._done = True
if raiseonerr and self.returncode:
self.log.error('Exit code: %s', self.returncode)
> raise subprocess.CalledProcessError(self.returncode, self.argv)
E CalledProcessError: Command '['kinit', 'admin']' returned non-zero exit status 1</pre></pre>
<br>
Similiarly for other tests. This is caused by the fact that you did
not set topology in the BaseTestAdvise class, like this:<br>
<br>
--- a/ipatests/test_integration/test_advise.py<br>
+++ b/ipatests/test_integration/test_advise.py<br>
@@ -31,6 +31,7 @@ class BaseTestAdvise(IntegrationTest, object):<br>
advice_id = None<br>
raiseerr = None<br>
advice_regex = ''<br>
+ topology = 'line'<br>
<br>
2.) BaseTestAdvise inherits from IntegrationTest and from object.
Explicitly specifying object as superclass is not needed,
IntegrationTest already inherits from it.<br>
<br>
3.) I think there is no good incentive to separate the test cases
into mutliple classes. Each test class adds overhead of installing
and uninstalling IPA server, to guarantee a clean and sane
environment. However, it seems to be an overkill for testing
ipa-advise command, which should be read-only anyway. By squashing
the tests into one test class, we will decrease the run time of this
test more than 8-fold.<br>
<br>
4.) The patch adds a whitespace error.<br>
<br>
The test cases themselves are looking fine, and when I fixed the
missing topology, they all passed. So this is a question of fixing
the above issues, and we should be ready to push.<br>
<br>
Tomas<br>
<br>
<div class="moz-cite-prefix">On 02/17/2015 03:29 PM, Gabe Alford
wrote:<br>
</div>
<blockquote
cite="mid:CAGLxfGwZ8BoCfzLj6Ay5R2oTOm_cejt6Ubbn84HTrPZSW4_dng@mail.gmail.com"
type="cite">Hello,
<div><br>
</div>
<div> I was wondering if I could get a review of this
patch. </div>
<div><br>
</div>
<div>Thanks,</div>
<div><br>
</div>
<div>Gabe<br>
<br>
On Thursday, January 29, 2015, Gabe Alford <<a
moz-do-not-send="true" href="mailto:redhatrises@gmail.com">redhatrises@gmail.com</a>>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div>
<div>Hello,<br>
<br>
</div>
<div> Here is a patch for <a moz-do-not-send="true"
href="https://fedorahosted.org/freeipa/ticket/4029"
target="_blank">https://fedorahosted.org/freeipa/ticket/4029</a>
I added test cases for valid and invalid advice.<br>
</div>
<div><br>
</div>
Thanks,<br>
</div>
<br>
Gabe<br>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Wed, Jan 14, 2015 at 10:23 AM,
Tomas Babej <span dir="ltr"><<a moz-do-not-send="true"
href="javascript:_e(%7B%7D,'cvml','tbabej@redhat.com');" target="_blank">tbabej@redhat.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div>
<div> <br>
<div>On 01/14/2015 06:13 PM, Gabe Alford wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">On Wed, Jan 14, 2015
at 10:05 AM, Tomas Babej <span dir="ltr"><<a
moz-do-not-send="true"
href="javascript:_e(%7B%7D,'cvml','tbabej@redhat.com');"
target="_blank">tbabej@redhat.com</a>></span>
wrote:<br>
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="margin:0 0 0 .8ex;border-left:1px
#ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"> <br>
<div>On 01/14/2015 06:00 PM, Tomas
Babej wrote:<br>
</div>
<blockquote type="cite">
<div>
<div> <br>
<div>On 01/14/2015 05:37 PM,
Tomas Babej wrote:<br>
</div>
<blockquote type="cite"> <br>
<div>On 01/14/2015 02:55 PM,
Gabe Alford wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>
<div>
<div>Hello,<br>
<br>
</div>
In looking into
<a
moz-do-not-send="true"
href="https://fedorahosted.org/freeipa/ticket/4029" target="_blank">https://fedorahosted.org/freeipa/ticket/4029</a>
I am wondering if
there should be
separate ipa-advise
test, Yes/No? Could be
handy in the future to
test more ipa-advise
output? Or should this
test be added to the
test_legacy_clients.py?<br>
<br>
</div>
Thanks,<br>
<br>
</div>
Gabe </div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On
Tue, Dec 2, 2014 at 9:21
PM, Gabe Alford <span
dir="ltr"><<a
moz-do-not-send="true"
href="javascript:_e(%7B%7D,'cvml','redhatrises@gmail.com');"
target="_blank">redhatrises@gmail.com</a>></span>
wrote:<br>
<blockquote
class="gmail_quote"
style="margin:0 0 0
.8ex;border-left:1px
#ccc
solid;padding-left:1ex">
<div dir="ltr">
<div>
<div>Hello,<br>
<br>
I was going to
try my hand at
attempting a
patch for
ipa-tests.
However in
wanting to test
my patch, I am
not sure how to
run ipa-tests to
check if it
works or not.
Documentation is
not really clear
on what needs to
be done to start
a test and run a
test. This is
for <a
moz-do-not-send="true"
href="https://fedorahosted.org/freeipa/ticket/4029" target="_blank">https://fedorahosted.org/freeipa/ticket/4029</a>
<br>
<br>
</div>
I have attached
the patch that I
have yet to really
test with
ipa-test. Any help
on how to test the
patch running
ipa-tests would be
great. Of course,
if one of the
reviewers looks at
the patch and
looks good, then I
would be happy
with that as well.<br>
<br>
</div>
Thanks,<br>
<br>
Gabe<br>
</div>
</blockquote>
</div>
<br>
</div>
<br>
<fieldset></fieldset>
<br>
<pre>_______________________________________________
Freeipa-devel mailing list
<a moz-do-not-send="true" href="javascript:_e(%7B%7D,'cvml','Freeipa-devel@redhat.com');" target="_blank">Freeipa-devel@redhat.com</a>
<a moz-do-not-send="true" href="https://www.redhat.com/mailman/listinfo/freeipa-devel" target="_blank">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
<br>
Hello,<br>
<br>
TL;DR: feel free to create a
separate ipa-advise test file.
Test requested in this ticket
really does not belong to the
legacy clients feature test.<br>
<br>
As for the any new tests that
might come: I think tests for
ipa-advise that are specific
to that particular feature
should be tested with that
feature, more so, if they
contain parts that are
supposed to work copy-pasted.
If a tests, however, tests a
general behaviour of
ipa-advise, it should live in
the ipa-advise namespace,
hence separate test file.<br>
<br>
HTH,<br>
<br>
<pre cols="72">--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | <a moz-do-not-send="true" href="http://freeipa.org" target="_blank">freeipa.org</a> </pre>
</blockquote>
<br>
</div>
</div>
The attached patch looks fine,
although, please also test for a
non-zero return code number.<br>
<br>
</blockquote>
<br>
Upon hitting send I noticed you did
not include raiseonerr=False into the
run_command call. You need to do that,
otherwise a exception will be raised,
since ipa-advise exited with non-zero
return code.<span><br>
</span></div>
</blockquote>
<div>Thanks Tomas.<br>
<br>
</div>
<div>Which do you prefer: a test_advise.py
or an update to the existing patch? <br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</div>
</div>
A new test file, as I pointed out in the second email
:) sorry for splitting.<br>
<br>
However, it would be the best if you could spin up a
positive test as well (maybe listing out available
advices), not just this negative one, to justify the
overhead reinstalling IPA for testing this feature.<span><br>
<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="margin:0 0 0 .8ex;border-left:1px
#ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"><span>
<blockquote type="cite"> <br>
<pre cols="72">--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | <a moz-do-not-send="true" href="http://freeipa.org" target="_blank">freeipa.org</a> </pre>
</blockquote>
<br>
<pre cols="72">--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | <a moz-do-not-send="true" href="http://freeipa.org" target="_blank">freeipa.org</a> </pre>
</span></div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</span><span>
<pre cols="72">--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | <a moz-do-not-send="true" href="http://freeipa.org" target="_blank">freeipa.org</a> </pre>
</span></div>
</blockquote>
</div>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>