Yeah. That makes more sense. Updated patch attached. Thanks,
Gabe On Wed, Feb 25, 2015 at 3:55 PM, Tomas Babej <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> 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 >> > > >
freeipa-rga-0039-3-ipatests-Add-tests-for-valid-and-invalid-ipa-advise.patch
Description: Binary data
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel