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

Ana Krivokapic akrivoka at redhat.com
Tue Jul 16 10:58:47 UTC 2013


On 07/16/2013 12:54 PM, Petr Vobornik wrote:
> 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.
>

ACK

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.




More information about the Freeipa-devel mailing list