[Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests
Petr Viktorin
pviktori at redhat.com
Tue Sep 17 08:43:17 UTC 2013
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³
More information about the Freeipa-devel
mailing list