[Freeipa-devel] [PATCH] 422-424 Web UI integration tests

[Petr Vobornik]

On 07/09/2013 05:37 PM, Ana Krivokapic wrote:
> On 06/21/2013 10:56 AM, Petr Vobornik wrote:
>> Sending an initial implementation of Web UI integration tests. The effort is
>> documented at http://www.freeipa.org/page/Web_UI_Integration_Tests .
>>
>> The coverage is not complete but rather basic. Sending it now as this is
>> ongoing task.
>>
>> Currently it tries to open all facets and dialogs and perform
>> add,mod,find,delete,add/remove associations for every entity except trusts (to
>> be implemented).
>>
>> First two attached patches are changing Web UI to be little more test-friendly
>> - they add some names to element and such. Tests are in the last patch.
>>
>> https://fedorahosted.org/freeipa/ticket/3744
>>

>
> I tried setting up and running the new tests, and overall it works really well.
> The documentation page is very clear and to the point, and the setup script is
> especially handy. Thanks!

Thanks for the review, comments bellow, updated patch 424 attached.

>
> Below are some comments (mostly related to the python code in ui_driver.py).
>
> - I got whitespace warnings when applying patch 424
> - ui_driver.py:110 - Should be 'except IOError, e', as you use 'e' within the
> except clause.
> - ui_driver.py:110 - There's too much code in the try block. We should have as
> little code as possible in the try block (ideally only the code that could
> actually raise the exception).

I assume you meant get_driver method on line ~150. The code on line 110 
consists of three lines. I've changed the block little bit but I'm not 
sure that it helped. The issue is that there are 4 calls which can raise 
the exceptions. Catching them separately would only clutter the code 
with exception handling.

> - ui_driver.py:200 - It says 'single' in the docstring, but the argument name is
> 'all'.
> - ui_driver.py:205 - Instead of raising ValueError, I would prefer something
> like: assert expression, 'expression is missing'
> - ui_driver.py:643 - There's a print statement, I assume a leftover from some
> debugging. :)
>

All other mentioned issues should be fixed.
-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0422-Better-automated-test-support.patch
Type: text/x-patch
Size: 5664 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130716/c22b832f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0423-Fix-container-element-in-adder-dialogs.patch
Type: text/x-patch
Size: 2757 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130716/c22b832f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0424-1-Upstream-Web-UI-tests.patch
Type: text/x-patch
Size: 146353 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130716/c22b832f/attachment-0002.bin>