[Freeipa-devel] [PATCH] 431-434 Web UI integration tests continuation

[Ana Krivokapic]

On 07/24/2013 05:33 PM, Petr Vobornik wrote:
> On 07/23/2013 05:50 PM, Ana Krivokapic wrote:
>> On 07/18/2013 01:35 PM, Petr Vobornik wrote:
>>> On 07/18/2013 01:34 PM, Petr Vobornik wrote:
>>>> [PATCH] 431 Web UI integration tests: Add trust tests
>>>>
>>>> [PATCH] 432 Web UI integration tests: Add ui_driver method descriptions
>>>>
>>>> [PATCH] 433 Web UI integration tests: Verify data after add and mod
>>>>
>>>> [PATCH] 434 Web UI integration tests: Compute range sizes to avoid overlaps
>>>>
>>>> Heavily inspired by code from xmlrpc tests.
>>>>
>>>> To obtain ranges, this patch also adds method to execute FreeIPA command
>>>> through Web UI.
>>>> It uses Web UI instead of ipalib so it doesn't need to care about
>>>> authentication on a test-runner machine.
>>>>
>>>> All: https://fedorahosted.org/freeipa/ticket/3744
>>>
>>> And now the patches...
>>>
>
>
> Thanks for the review.
>
>> Patch 431:
>> - In case 'ipa-adtrust-install' has not been run on the system, but 'has_trusts'
>> is configured in 'ui_test.conf', trust tests fail. I suggest checking for
>> 'ipa-adtrust-install' with 'adtrust_is_enabled' command and then skipping tests
>> if trusts are not enabled.
>
> IMO it's correct behavior - you want to test the feature, but server is
> misconfigured - fail.

OK, agreed.

>
>>
>> Patch 432:
>> - Docstrings for 'has_ca()' and 'has_dns()' state that 'FreeIPA server was
>> installed *without* CA/DNS' when they shoud state *with*.
>>
>> Patch 433:
>> - Please also add docstrings for newly added methods.
>> - The long 'if' statement in 'validate_fields()' can be shortened by using the
>> 'in' keyword instead of individual string comparisons. For example:
>>      elif ftype in ('textbox', 'password', 'combobox'):
>>          actual = self.get_field_value(key, parent, 'input')
>> - IIUC, the code in validate_fields() compares lists irrespective of element
>> order. If this is the case, it can be replaced by simply 'sorted(expected) ==
>> sorted(actual)'.
>> - In 'basic_crud()', shouldn't validate_fields with 'add_v' be called right
>> after 'add_record'? The way it is now, data only gets validated in case of data
>> modification, but not after initial addition.
>>
>> Patch 434:
>> - The "if 'ipasecondarybaserid' in idrange" block should be nested under the "if
>> 'ipasecondarybaserid' in idrange" block, since it assumes that the 'base_rid'
>> variable is set.
>> - Please remove trailing semicolons.
>
> Fixed
>
>>
>>
>> General comments (these comments are by no means a requirement, and they are not
>> a reason for a NACK, just a "nice to have"):
>> - Some methods have unused parameters (e.g. validate_fields, basic_crud)
>> - Some methods define variables which shadow Python built-ins of the same name
>
> New patch #438.
>
>> (e.g. type, all). This can be a problem if you later want to use the built-in.
>> - It would be nice to make at least newly added code PEP8 compliant. The 'pep8'
>> utility can be used to easily check any python file for PEP8 compliance:
>>      $ sudo yum install python-pep8
>>      $ pep8 /path/to/script.py
>>
>
> Attaching new patch #437 which fixes PEP8 errors (except long lines). Most of
> the work was done automatically by autopep8 tool.

Thanks for the fixes. ACK on all 6 patches.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.