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