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

Thu Sep 19 10:14:38 UTC 2013

On 09/17/2013 04:35 PM, Tomas Babej wrote:
> 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.
>

I actually noticed that some of the fixes that address issues above 
stayed hidden in my git stash.

Updated patches attached.

-- 
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-3-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/20130919/4e0bf876/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0101-3-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/20130919/4e0bf876/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0102-3-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/20130919/4e0bf876/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0103-3-ipatests-Add-WinHost-class.patch
Type: text/x-patch
Size: 3024 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130919/4e0bf876/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0104-3-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/20130919/4e0bf876/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0105-3-ipatests-Add-AD-integration-related-tasks.patch
Type: text/x-patch
Size: 6930 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130919/4e0bf876/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0106-3-ipatests-Add-AD-integration-test-case.patch
Type: text/x-patch
Size: 6844 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130919/4e0bf876/attachment-0006.bin>