On 09/16/2013 03:45 PM, Tomas Babej wrote:
Hi,
this set of patches extends ipatests module to support integration
testing with Active Directory,
as well as provides an basic (working without artificial sleeps!) trust
test case.
Thanks, this is great news!
I haven't testes the test yet. Here are my comments from reading them.
Patch 100:
Please document the new configuration options. There are two places:
- ipa-test-config man page
- http://www.freeipa.org/page/V3/Integration_testing#Test_configuration
(or if you end up writing a separate design page for AD tests, link to it)
Patch 101:
The `if role == 'ad':` block should rather be in the `Host.from_env`
factory.
Patch 102:
Thanks for cleaning that up!
You could go one step further with
cls.replicas = get_resources(domain.replicas, 'replicas', cls.num_replicas)
and `return container[:num_needed]` there
Patch 103:
This patch should come before 101 which uses it.
Ideally there would be a BaseHost with common functionality, and
concrete Host and WinHost subclassing it.
I'll be making changes here for #3890; please concentrate on other parts
for now to avoid conflicts. I'll take Windows hosts into consideration.
Raise instances rather than the exception classes: raise
NotImplementedError()
Patch 104:
Instead of stdout_re, allow the user to pass in a checking function.
For example, `lambda result: re.search(sid_regex, result.stdout_text)`
This makes wait_assert complete, you won't need to add other cases to it
when you need to check stderr or evaluate some condition a regex is too
weak for.
Pass `raiseonerr` to run_command directly, not by setting it in kwargs.
That way wait_assert will fail if it gets raiseonerr.
Also, run_command's arguments except `command` are supposed to be
keyword-only. (This is only not enforced because that's awkward to do in
Python 2.) Don't accept or pass along *args.
Use parentheses instead of \ for line continuation (see PEP8).
I'd prefer to keep the function in test_trust.py until we see there's a
wider scope for it.
And rename it to run_repeatedly or some other name that describes it better.
Patch 105:
Please add these tasks to ipa-test-task (and its man page) as well.
The instructions in the docstring in configure_dns_for_trust should go
to a test design document. I think just starting a section in
Integration_testing would be fine if you mark it as not implemented yet
(and link to the ticket).
Use parentheses instead of \ for line continuation (see PEP8).
Patch 106:
The instructions in the TestADTrust docstring should go to a test design
document.
Please use the SID regex from a shared location. You'll need to assign
it to a variable and make the Fuzzy from that, so that variable can be
imported and used with the standard re module.
Avoid commented-out code in patches for review.
No need to import fuzzy.
test_user_gid_uid_resolution_in_nonposix_trust:
- For a one-off regex, compile() is unnecessary:
assert re.search(testuser_regex, result.stdout_text)
- Whenever substituting a literal string into a regex, please use
re.escape().
Use parentheses instead of \ for line continuation (see PEP8).
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
