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

[Petr Vobornik]

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.

>
> 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.
-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0431-1-Web-UI-integration-tests-Add-trust-tests.patch
Type: text/x-patch
Size: 7886 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130724/50b6c675/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0432-1-Web-UI-integration-tests-Add-ui_driver-method-descri.patch
Type: text/x-patch
Size: 10503 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130724/50b6c675/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0433-1-Web-UI-integration-tests-Verify-data-after-add-and-m.patch
Type: text/x-patch
Size: 6804 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130724/50b6c675/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0434-1-Web-UI-integration-tests-Compute-range-sizes-to-avoi.patch
Type: text/x-patch
Size: 4701 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130724/50b6c675/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0437-Web-UI-integration-tests-PEP8-fixes.patch
Type: text/x-patch
Size: 45071 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130724/50b6c675/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0438-Web-UI-integration-tests-Code-quality-fixes.patch
Type: text/x-patch
Size: 9132 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130724/50b6c675/attachment-0005.bin>