On 07/16/2013 10:52 AM, Petr Vobornik wrote:
> 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.
Yes, that's what I meant. Sorry about the typo. It looks better now, thanks.
>
>> - 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.
Just one more nitpick (sorry for not catching it the first time). I don't see a
reason for nested ifs here:
if browser == 'chrome' or browser == 'chromium':
capabilities = DesiredCapabilities.CHROME
if browser == 'chromium':
capabilities = options.to_capabilities()
elif browser == 'ie':
capabilities = DesiredCapabilities.INTERNETEXPLORER
else:
capabilities = DesiredCapabilities.FIREFOX
It can be written simply as:
if browser == 'chrome':
...
elif browser == 'chromium':
...
elif browser == 'ie':
...
else:
...
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
