On 07/23/2013 05:50 PM, Ana Krivokapic wrote:
On 07/18/2013 01:35 PM, Petr Vobornik wrote:
On 07/18/2013 01:34 PM, Petr Vobornik wrote:
[PATCH] 431 Web UI integration tests: Add trust tests
[PATCH] 432 Web UI integration tests: Add ui_driver method descriptions
[PATCH] 433 Web UI integration tests: Verify data after add and mod
[PATCH] 434 Web UI integration tests: Compute range sizes to avoid
overlaps
Heavily inspired by code from xmlrpc tests.
To obtain ranges, this patch also adds method to execute FreeIPA command
through Web UI.
It uses Web UI instead of ipalib so it doesn't need to care about
authentication on a test-runner machine.
All: https://fedorahosted.org/freeipa/ticket/3744
And now the patches...
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Patch 431:
- In case 'ipa-adtrust-install' has not been run on the system, but
'has_trusts' is configured in 'ui_test.conf', trust tests fail. I
suggest checking for 'ipa-adtrust-install' with 'adtrust_is_enabled'
command and then skipping tests if trusts are not enabled.
Patch 432:
- Docstrings for 'has_ca()' and 'has_dns()' state that 'FreeIPA server
was installed *without* CA/DNS' when they shoud state *with*.
Patch 433:
- Please also add docstrings for newly added methods.
- The long 'if' statement in 'validate_fields()' can be shortened by
using the 'in' keyword instead of individual string comparisons. For
example:
elif ftype in ('textbox', 'password', 'combobox'):
actual = self.get_field_value(key, parent, 'input')
- IIUC, the code in validate_fields() compares lists irrespective of
element order. If this is the case, it can be replaced by simply
'sorted(expected) == sorted(actual)'.
- In 'basic_crud()', shouldn't validate_fields with 'add_v' be called
right after 'add_record'? The way it is now, data only gets validated in
case of data modification, but not after initial addition.
Patch 434:
- The "if 'ipasecondarybaserid' in idrange" block should be nested under
the "if 'ipasecondarybaserid' in idrange" block, since it assumes that
the 'base_rid' variable is set.
- Please remove trailing semicolons.
General comments (these comments are by no means a requirement, and they
are not a reason for a NACK, just a "nice to have"):
- Some methods have unused parameters (e.g. validate_fields, basic_crud)
- Some methods define variables which shadow Python built-ins of the
same name (e.g. type, all). This can be a problem if you later want to
use the built-in.
- It would be nice to make at least newly added code PEP8 compliant. The
'pep8' utility can be used to easily check any python file for PEP8
compliance:
$ sudo yum install python-pep8
$ pep8 /path/to/script.py
It can also check only your changes:
git diff -U0 origin/master.. | pep8 --diff
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
