<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
ACK.<br>
<br>
Pushed to:<br>
ipa-4-1: ddd7fb6a68fd413b1561eab9c29bac18882e5efd<br>
master: ae4ee6b53376bb7f3d1b4707c4e105c91b5cd8ab<br>
<br>
<div class="moz-cite-prefix">On 02/26/2015 05:58 PM, Gabe Alford
wrote:<br>
</div>
<blockquote
cite="mid:CAGLxfGz8w=SdLXQYt8aW5_Gy11d7E8H2e-H=XxeFYpR5+7PBzg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>Yeah. That makes more sense. Updated patch attached.<br>
<br>
</div>
Thanks,<br>
<br>
</div>
Gabe<br>
<div>
<div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Wed, Feb 25, 2015 at 3:55 PM,
Tomas Babej <span dir="ltr"><<a
moz-do-not-send="true"
href="mailto: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"> Hi Gabe,<br>
<br>
sorry for not being clear. This approach will not
work:<br>
<br>
+class TestAdvice(BaseTestInvalidAdvice,<br>
+ BaseTestFedoraAuthconfig,<br>
+ BaseTestFreeBSDNSSPAM,<br>
+ BaseTestGenericNSSPAM,<br>
+ BaseTestGenericSSSDBefore19,<br>
+ BaseTestRedHatNSS,<br>
+ BaseTestRedHatNSSPAM,<br>
+ BaseTestRedHatSSSDBefore19,<br>
+ BaseTestAdvice):<br>
+ pass<br>
<br>
By combining all the base classes into one, you will
not get the desired effect (which is to run the
test_advice method for each advice_id). Let me
explain why:<br>
<br>
The test runner works in the following way: it
inspects any discovered class which name begins with
"Test", and executes each its method, which names
begins with "test" as a test case.<br>
<br>
If the test runner inspects the TestAdvice class,
the only method beggining with "test", which it will
see, is the "test_advice" which was inherited back
from BaseTestAdvice class. So we can safely conclude
the test runner will only run 1 test case.<br>
<br>
Which one, you may ask? Well, since the test_advice
behaviour is fully determined by the values of
"advice_id", "advice_regex" and "raiseerr"
attributes, let's look at their values in TestAdvice
class. This class does not define attirbutes with
such names, so we move along the inheritance chain
(also called MRO) - the first class from which we
inherit is BaseTestInvalidAdvice, and this class
defines all three mentioned attributes.<br>
<br>
Hence the only test method will be run the test for
invalid advice :)<br>
<br>
Now, how to fix this? The easiest approach would be
to abandon the approach with the separate classes,
and map each class to a test method in the
TestAdvice class, like this (from the top of my
head):<br>
<br>
+class TestAdvice(IntegrationTest):<br>
+ topology = 'line'<br>
+<br>
+ def test_invalid_advice(self):<br>
+ advice_id = 'invalid-advise-param'<br>
+ advice_regex = "invalid[\s]+\'advice\'.*"<br>
+ raiseerr = False<br>
+ # Obtain the advice from the server<br>
+ tasks.kinit_admin(self.master)<br>
+ result =
self.master.run_command(['ipa-advise',
self.advice_id],<br>
+
raiseonerr=self.raiseerr)<br>
+<br>
+ if not result.stdout_text:<br>
+ advice = result.stderr_text<br>
+ else:<br>
+ advice = result.stdout_text<br>
+<br>
+ assert re.search(self.advice_regex, advice,
re.S)<br>
+<br>
+ def test_advice_fedora_authconfig(self):<br>
+ advice_id = 'config-fedora-authconfig'<br>
+ advice_regex = "\#\!\/bin\/sh.*" \<br>
+
"authconfig[\s]+\-\-enableldap[\s]+" \<br>
+
"\-\-ldapserver\=.*[\s]+\-\-enablerfc2307bis[\s]+" \<br>
+ "\-\-enablekrb5"<br>
+ raiseonerr = True<br>
+ # Obtain the advice from the server<br>
+ tasks.kinit_admin(self.master)<br>
+ result =
self.master.run_command(['ipa-advise',
self.advice_id],<br>
+
raiseonerr=self.raiseerr)<br>
+<br>
+ if not result.stdout_text:<br>
+ advice = result.stderr_text<br>
+ else:<br>
+ advice = result.stdout_text<br>
+<br>
+ assert re.search(self.advice_regex, advice,
re.S)<br>
<br>
... the same for the remaining 6 cases<br>
<br>
Now, this pattern has lots of duplicated code which
can be extracted to a helper method, I just thought
it would help to be more explicit to get the idea
across. In the end you can achieve the same level of
conciseness than with the separate test classes.
Good luck!<br>
<br>
HTH,<br>
<br>
Tomas
<div>
<div class="h5"><br>
<br>
<br>
<br>
<div>On 02/25/2015 03:52 PM, Gabe Alford wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">No worries about
the delay. Thanks for taking the time!
Updated patch attached.<br>
<br>
</div>
<div class="gmail_quote">Thanks,<br>
<br>
</div>
<div class="gmail_quote">Gabe<br>
</div>
<div class="gmail_quote"><br>
On Tue, Feb 24, 2015 at 11:03 AM, Tomas
Babej <span dir="ltr"><<a
moz-do-not-send="true"
href="mailto:tbabej@redhat.com"
target="_blank">tbabej@redhat.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div> Hi Gabe,<br>
<br>
sorry for the delay. Here comes the
review! <br>
</div>
</blockquote>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
1.) All the tests fail, since the
IPA master is not installed at all:<br>
<br>
<pre><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.<span><font color="#888888"><br>
<br>
Tomas</font></span><br>
<span></span></div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>