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

Fri Sep 27 11:12:52 UTC 2013

On 09/19/2013 05:58 PM, Petr Viktorin wrote:
> 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.
>
> Looks good.
>
>>> Patch 101:
>>> The `if role == 'ad':` block should rather be in the `Host.from_env`
>>> factory.
>>>
>>
>> Moved.
>
> Looks good.
> With my patches for #3890, this should use BaseHost.from_env.

Fixed.

>
>>> 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.
>
> Looks good.
>
>>> 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).
>
> Sure.
>
>>> 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.
>
> My patches are on the list. Conflicts should be minimal; withthem 
> WinHost should end up empty.

My patches are now rebased on top of yours.

>
>>> Raise instances rather than the exception classes: raise
>>> NotImplementedError()
>> Fixed.
>
>>> Patch 104:
> [...]
>>> 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.
>
> Thanks
>
>> 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.
>
> I'd like to keep tasks to high-level operations, the kind that 
> ipa-test-task would do.
> (Putting wait_for_replication there was, in hindsight, a mistake.)
>
> We can add an util module later. It's easy to move things to a shared 
> module when needed, but if we do it too early and often the module 
> will be hard to maintain. Predicting the future in this respect hasn't 
> worked very well for me :)

I moved that to the util.py. The reason is that this function is also 
used from within AD integration tasks. Having the tasks themselves 
import functions from the testcases does not seem right.

Do you want to move wait_for_replication there as well?

>
>>> 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).
>
> This still needs some work (unfortunately I haven't found a way to 
> make it less tedious).

I added the man page section and removed docstrings.

>
>>> 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).
>
> Personal opinions aside, the PEP8 is pretty clear on this, sorry. It 
> lists `with` and `assert` statements where you can't use parentheses 
> around everything after the keyword, but that's not case here.
>

Fixed.

>>> 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.
>
> OK, let's worry about this later.
>
> [...]
>>> 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.
>
> Sorry, I meant non-literal and wrote exactly the opposite! This is 
> suboptimal:
>     self.ad.domain.name.replace('.', '\.')
> This is better:
>     re.escape(self.ad.domain.name)

Fixed.

>
>>
>>> Use parentheses instead of \ for line continuation (see PEP8).
>>>
>>>
>>
>> Design will follow soon.
>
> Perfect!
>
>

Design page draft:

http://www.freeipa.org/page/V3/Integration_testing/AD

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130927/a37305e9/attachment.htm>
-------------- 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/20130927/a37305e9/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: 3151 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130927/a37305e9/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/20130927/a37305e9/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: 2506 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130927/a37305e9/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0104-3-ipatests-Create-util-module-for-ipatests.patch
Type: text/x-patch
Size: 1721 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130927/a37305e9/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: 9163 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130927/a37305e9/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: 5630 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130927/a37305e9/attachment-0006.bin>