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

[Petr Viktorin]

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.

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

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

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

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

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

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

Perfect!


-- 
Petr³