[Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests

[Petr Viktorin]

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³