Yeah. That makes more sense. Updated patch attached.
Thanks,
Gabe
On Wed, Feb 25, 2015 at 3:55 PM, Tomas Babej <tba...@redhat.com
<mailto:tba...@redhat.com>> wrote:
Hi Gabe,
sorry for not being clear. This approach will not work:
+class TestAdvice(BaseTestInvalidAdvice,
+ BaseTestFedoraAuthconfig,
+ BaseTestFreeBSDNSSPAM,
+ BaseTestGenericNSSPAM,
+ BaseTestGenericSSSDBefore19,
+ BaseTestRedHatNSS,
+ BaseTestRedHatNSSPAM,
+ BaseTestRedHatSSSDBefore19,
+ BaseTestAdvice):
+ pass
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:
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.
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.
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.
Hence the only test method will be run the test for invalid advice :)
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):
+class TestAdvice(IntegrationTest):
+ topology = 'line'
+
+ def test_invalid_advice(self):
+ advice_id = 'invalid-advise-param'
+ advice_regex = "invalid[\s]+\'advice\'.*"
+ raiseerr = False
+ # Obtain the advice from the server
+ tasks.kinit_admin(self.master)
+ result = self.master.run_command(['ipa-advise',
self.advice_id],
+ raiseonerr=self.raiseerr)
+
+ if not result.stdout_text:
+ advice = result.stderr_text
+ else:
+ advice = result.stdout_text
+
+ assert re.search(self.advice_regex, advice, re.S)
+
+ def test_advice_fedora_authconfig(self):
+ advice_id = 'config-fedora-authconfig'
+ advice_regex = "\#\!\/bin\/sh.*" \
+ "authconfig[\s]+\-\-enableldap[\s]+" \
+ "\-\-ldapserver\=.*[\s]+\-\-enablerfc2307bis[\s]+" \
+ "\-\-enablekrb5"
+ raiseonerr = True
+ # Obtain the advice from the server
+ tasks.kinit_admin(self.master)
+ result = self.master.run_command(['ipa-advise',
self.advice_id],
+ raiseonerr=self.raiseerr)
+
+ if not result.stdout_text:
+ advice = result.stderr_text
+ else:
+ advice = result.stdout_text
+
+ assert re.search(self.advice_regex, advice, re.S)
... the same for the remaining 6 cases
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!
HTH,
Tomas
On 02/25/2015 03:52 PM, Gabe Alford wrote:
No worries about the delay. Thanks for taking the time! Updated
patch attached.
Thanks,
Gabe
On Tue, Feb 24, 2015 at 11:03 AM, Tomas Babej <tba...@redhat.com
<mailto:tba...@redhat.com>> wrote:
Hi Gabe,
sorry for the delay. Here comes the review!
1.) All the tests fail, since the IPA master is not installed
at all:
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
Similiarly for other tests. This is caused by the fact that
you did not set topology in the BaseTestAdvise class, like this:
--- a/ipatests/test_integration/test_advise.py
+++ b/ipatests/test_integration/test_advise.py
@@ -31,6 +31,7 @@ class BaseTestAdvise(IntegrationTest, object):
advice_id = None
raiseerr = None
advice_regex = ''
+ topology = 'line'
2.) BaseTestAdvise inherits from IntegrationTest and from
object. Explicitly specifying object as superclass is not
needed, IntegrationTest already inherits from it.
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.
4.) The patch adds a whitespace error.
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.
Tomas
