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

[Ana Krivokapic]

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...
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
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.

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.


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
(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

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130723/92a9954e/attachment.htm>