<html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 09/19/2013 05:58 PM, Petr Viktorin wrote: </div> <blockquote cite="mid:523B1F0F.3010101@redhat.com" type="cite">On 09/17/2013 04:35 PM, Tomas Babej wrote: <blockquote type="cite">On 09/17/2013 10:43 AM, Petr Viktorin wrote: <blockquote type="cite">On 09/16/2013 03:45 PM, Tomas Babej wrote: <blockquote type="cite">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. </blockquote> 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 - <a class="moz-txt-link-freetext" href="http://www.freeipa.org/page/V3/Integration_testing#Test_configuration">http://www.freeipa.org/page/V3/Integration_testing#Test_configuration</a> (or if you end up writing a separate design page for AD tests, link to it) </blockquote> I added the options to the the man page. </blockquote> Looks good. <blockquote type="cite"> <blockquote type="cite">Patch 101: The `if role == 'ad':` block should rather be in the `Host.from_env` factory. </blockquote> Moved. </blockquote> Looks good. With my patches for #3890, this should use BaseHost.from_env. </blockquote> Fixed. <blockquote cite="mid:523B1F0F.3010101@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite">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 </blockquote> You're welcome, refactoring part expanded. </blockquote> Looks good. <blockquote type="cite"> <blockquote type="cite">Patch 103: This patch should come before 101 which uses it. </blockquote> 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). </blockquote> Sure. <blockquote type="cite"> <blockquote type="cite">Ideally there would be a BaseHost with common functionality, and concrete Host and WinHost subclassing it. </blockquote> Yes, I agree, that's what we discussed. <blockquote type="cite">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. </blockquote> </blockquote> My patches are on the list. Conflicts should be minimal; withthem WinHost should end up empty. </blockquote> My patches are now rebased on top of yours. <blockquote cite="mid:523B1F0F.3010101@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite">Raise instances rather than the exception classes: raise NotImplementedError() </blockquote> Fixed. </blockquote> <blockquote type="cite"> <blockquote type="cite">Patch 104: </blockquote> </blockquote> [...] <blockquote type="cite"> <blockquote type="cite">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. </blockquote> I renamed the function. </blockquote> Thanks <blockquote type="cite">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. </blockquote> 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 :) </blockquote> 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? <blockquote cite="mid:523B1F0F.3010101@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite">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). </blockquote> </blockquote> This still needs some work (unfortunately I haven't found a way to make it less tedious). </blockquote> I added the man page section and removed docstrings. <blockquote cite="mid:523B1F0F.3010101@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite">Use parentheses instead of \ for line continuation (see PEP8). </blockquote> 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). </blockquote> 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. </blockquote> Fixed. <blockquote cite="mid:523B1F0F.3010101@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite">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. </blockquote> 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. </blockquote> OK, let's worry about this later. [...] <blockquote type="cite"> <blockquote type="cite">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(). </blockquote> Fixed. </blockquote> 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) </blockquote> Fixed. <blockquote cite="mid:523B1F0F.3010101@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite">Use parentheses instead of \ for line continuation (see PEP8). </blockquote> Design will follow soon. </blockquote> Perfect! </blockquote> Design page draft: <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1"> <a href="http://www.freeipa.org/page/V3/Integration_testing/AD">http://www.freeipa.org/page/V3/Integration_testing/AD</a> <pre class="moz-signature" cols="72">-- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org</pre> </body> </html>