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³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
