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. _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
