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

Tomas Babej tbabej at redhat.com
Tue Sep 17 14:35:02 UTC 2013


On 09/17/2013 10:43 AM, Petr Viktorin wrote:
> 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)
>

I added the options to the the man page.

>
> Patch 101:
> The `if role == 'ad':` block should rather be in the `Host.from_env` 
> factory.
>

Moved.

>
> 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

You're welcome, refactoring part expanded.

>
>
> Patch 103:
> This patch should come before 101 which uses it.
>

We can push this in the correct order if this is an issue, right? 
Patches are independent (meaning they do not touch the same source code, 
so they should apply cleanly).

> Ideally there would be a BaseHost with common functionality, and 
> concrete Host and WinHost subclassing it.

Yes, I agree, that's what we discussed.

> 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()
Fixed.

>
>
> 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.
+1, improved.

>
> 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.
>
Agreed, fixed.

> 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.
>

I renamed the function.

Why do you think there's not scope for it? I'd rather keep it in tasks, 
since it addresses
a general use case of waiting in a loop until a command returns expected 
output.

This can happen with any command that starts asynchronous actions and we 
want to make
sure that the changes are propagated before continuing (such as DNS 
updates via our CLI).

I wouldn't consider this being specific for AD trust testing.

>
>
> 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).

I don't really like wrapping arbitrary expressions in parentheses, 
particularly when assigning the result to a variable:

result = (something or
               something_completely_else)

To me, the following looks better:

result = something or \
              something completely else

I checked with PEP8. I remember there was a section that backslash can 
be used in cases it produces nicer results, but it's gone now.
Backslash is still recommended in some cases.

Still (but no strong opinions here), I would not consider this a 
violation unless it happens between brackets (in which case it is 
redundant).

>
> 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.
>

This is something that I tried to address with the Fuzzy refactoring. 
This is a temporary solution, once we resolve that patchset, of course, 
the test will use the shared location.
I added a comment there, I'd rather not create conflicts.

> Avoid commented-out code in patches for review.
> No need to import fuzzy.

Fixed.

>
> 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().
>
Fixed.

> Use parentheses instead of \ for line continuation (see PEP8).
>
>

Design will follow soon.

-- 
Tomas Babej
Associate Software Engeneer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0100-2-ipatests-Add-Active-Directory-support-to-configurati.patch
Type: text/x-patch
Size: 5280 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130917/8c1bb8ae/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0101-2-ipatests-Extend-domain-object-with-ad-role-support-a.patch
Type: text/x-patch
Size: 3078 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130917/8c1bb8ae/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0102-2-ipatests-Extend-IntegrationTest-with-multiple-AD-dom.patch
Type: text/x-patch
Size: 2786 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130917/8c1bb8ae/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0103-2-ipatests-Add-WinHost-class.patch
Type: text/x-patch
Size: 3024 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130917/8c1bb8ae/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0104-2-ipatests-Add-wait_assert-method-to-tasks-suite.patch
Type: text/x-patch
Size: 1813 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130917/8c1bb8ae/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0105-2-ipatests-Add-AD-integration-related-tasks.patch
Type: text/x-patch
Size: 6859 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130917/8c1bb8ae/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0106-2-ipatests-Add-AD-integration-test-case.patch
Type: text/x-patch
Size: 6817 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130917/8c1bb8ae/attachment-0006.bin>


More information about the Freeipa-devel mailing list