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

[Petr Viktorin]

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...
>>
>>
>> _______________________________________________
>> 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


It can also check only your changes:

git diff -U0 origin/master.. | pep8 --diff




-- 
Petr³