<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">No worries about the delay. Thanks for taking the time! Updated patch attached.<br><br></div><div class="gmail_quote">Thanks,<br><br></div><div class="gmail_quote">Gabe<br></div><div class="gmail_quote"><br>On Tue, Feb 24, 2015 at 11:03 AM, Tomas Babej <span dir="ltr"><<a href="mailto:tbabej@redhat.com" target="_blank">tbabej@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  
    
  
  <div>
    Hi Gabe,<br>
    <br>
    sorry for the delay. Here comes the review! <br></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
    1.) All the tests fail, since the IPA master is not installed at
    all:<br>
    <br>
    <pre><pre>    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</pre></pre>
    <br>
    Similiarly for other tests. This is caused by the fact that you did
    not set topology in the BaseTestAdvise class, like this:<br>
    <br>
    --- a/ipatests/test_integration/test_advise.py<br>
    +++ b/ipatests/test_integration/test_advise.py<br>
    @@ -31,6 +31,7 @@ class BaseTestAdvise(IntegrationTest, object):<br>
         advice_id = None<br>
         raiseerr = None<br>
         advice_regex = ''<br>
    +    topology = 'line'<br>
    <br>
    2.) BaseTestAdvise inherits from IntegrationTest and from object.
    Explicitly specifying object as superclass is not needed,
    IntegrationTest already inherits from it.<br>
    <br>
    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.<br>
    <br>
    4.) The patch adds a whitespace error.<br>
    <br>
    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.<span class=""><font color="#888888"><br>
    <br>
    Tomas</font></span><br><span class=""></span></div>

</blockquote></div><br></div></div>