[Freeipa-devel] [PATCH] 0057 Add integration tests for Kerberos Flags
Ana Krivokapic
akrivoka at redhat.com
Tue Aug 27 10:25:08 UTC 2013
On 08/27/2013 10:45 AM, Petr Viktorin wrote:
> 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,
>
>
Thanks for the review!
All good points, all fixed.
Updated patch is attached.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0057-02-Add-integration-tests-for-Kerberos-Flags.patch
Type: text/x-patch
Size: 8251 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130827/9cc90ce3/attachment.bin>
More information about the Freeipa-devel
mailing list