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

[Petr Vobornik]

On 07/16/2013 12:33 PM, Ana Krivokapic wrote:
> 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:
>          ...
>

You are right. Fixed. Patch attached.

-- 
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/e844c609/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/e844c609/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0424-2-Upstream-Web-UI-tests.patch
Type: text/x-patch
Size: 146322 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130716/e844c609/attachment-0002.bin>