[Freeipa-devel] [PATCH] 0057 Add integration tests for Kerberos Flags
Petr Viktorin
pviktori at redhat.com
Tue Aug 27 10:48:06 UTC 2013
On 08/27/2013 12:25 PM, Ana Krivokapic wrote:
> 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.
>
Thanks, ACK, pushed to
master: 1749cce3f7fd5874a32102c4d0e2c45964e9cf15
ipa-3-3: b0b25a27cf77c0b9b2aaffaceccb83fd1bfd6b54
--
Petr³
More information about the Freeipa-devel
mailing list