[Freeipa-devel] [PATCH] 0057 Add integration tests for Kerberos Flags

[Petr Viktorin]

On 08/25/2013 08:56 PM, Ana Krivokapic wrote:
> Hello,
>
> This patch adds integration tests for the Kerberos Flags feature (except the web
> UI tests), according to the test plan at:
> http://www.freeipa.org/page/V3/Kerberos_Flags#Test_Plan.
>
> https://fedorahosted.org/freeipa/ticket/3831

Thank you! The tests work well. I do have a few nitpicks though.


[...]
> +class TestKerberosFlags(IntegrationTest):
> +    """
> +    Test Kerberos Flags
> +http://www.freeipa.org/page/V3/Kerberos_Flags#Test_Plan
> +    """
> +    topology = 'line'
> +    num_clients = 1
> +
> +    def test_set_flag_with_host_add(self):
> +        host = 'host.example.com'
> +        host_service = 'host/host.example.com'
> +        host_keytab = '/tmp/host.keytab'

This function has three nearly identical blocks. Why not say `for 
trusted in (True, False, None):` here?
Same for the others.

[...]
> +        result = self.master.run_command(args)
> +        assert result.returncode == 0

These asserts are not needed; run_command() checks automatically unless 
you specify raise_on_error.

[...]
> +    def check_flag_klist(self, service, trusted):
> +        result = self.master.run_command(['klist', '-f'])
> +        output_lines = result.stdout_text.split('\n')
> +        flags = ''
> +
> +        for line in output_lines:
> +            if service in line:
> +                i = output_lines.index(line)

For looping with an index you should generally use `for i, line in 
enumerate(output_lines):`
In this specific case, you can use the "sliding window iteration" idiom:

     for line, next_line in zip(output_lines, output_lines[1:]):

> +                flags = output_lines[i+1].replace('Flags:', '').strip()
> +
> +        if trusted:
> +            assert 'O' in flags
> +        else:
> +            assert 'O' not in flags
> +
> +    def rekinit(self):
> +        self.master.run_command(['kdestroy'])
> +        tasks.kinit_admin(self.master)
> +
> +    def getkeytab(self, service, keytab):
> +        result = self.master.run_command([
> +            'ipa-getkeytab',
> +            '-s',
> +            self.master.hostname,
> +            '-p',
> +            service,
> +            '-k',
> +            keytab

A really minor one -- I find the following more readable:

     'ipa-getkeytab',
     '-s', self.master.hostname,
     '-p', service,
     '-k', keytab,


-- 
Petr³