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