[Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise
Tomas Babej
tbabej at redhat.com
Wed Feb 25 22:55:55 UTC 2015
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 <tbabej at redhat.com
> <mailto:tbabej at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150225/c2569b08/attachment.htm>
More information about the Freeipa-devel
mailing list