[Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

[Gabe Alford]

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> 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/eb08c747/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rga-0039-2-ipatests-Add-tests-for-valid-and-invalid-ipa-advise.patch
Type: application/octet-stream
Size: 5467 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150225/eb08c747/attachment.obj>