<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 27.10.2015 10:22, Martin Basti wrote: </div> <blockquote cite="mid:562F424D.3060003@redhat.com" type="cite"> On 27.10.2015 10:00, Oleg Fayans wrote: <blockquote type="cite">Hi Martin, The updated version of the patch is attached. Please, see my comments below </blockquote> My comments inline, I may be completely wrong in how the test suite work, so feel free to correct me. Martin <blockquote type="cite"> On 10/26/2015 06:48 PM, Martin Basti wrote: <blockquote type="cite"> On 26.10.2015 08:59, Oleg Fayans wrote: <blockquote type="cite"> On 10/23/2015 03:10 PM, Martin Basti wrote: <blockquote type="cite"> On 23.10.2015 15:00, Oleg Fayans wrote: <blockquote type="cite">Hi Martin, Here comes the updated version. On 10/22/2015 05:38 PM, Martin Basti wrote: <blockquote type="cite"> On 22.10.2015 15:23, Martin Basti wrote: <blockquote type="cite"> On 22.10.2015 14:13, Oleg Fayans wrote: <blockquote type="cite"> </blockquote> Hello, thank you for the patch. 1) please remove the added empty lines, they are unrelated to this ticket </blockquote> </blockquote> done <blockquote type="cite"> <blockquote type="cite"> 2) -def install_master(host, setup_dns=True, setup_kra=False): +def install_master(host, setup_dns=True, setup_kra=False, domainlevel=1): I suggest to use default domainlevel=None, which will use the default domain level (specified in build) </blockquote> </blockquote> done <blockquote type="cite"> <blockquote type="cite"> 3) + domain_level = domainlevel(master) I do not think that this meets expectations. We have to test, both domain level 0 and 1 for IPA 4.3, respectively new IPA must support all older domain levels, domain level is independent on IPA version, only admin can raise it up. So you have to find out way how to pass the domain level for which test will be running, we were talking about using config files, but feel free to find something new and better </blockquote> </blockquote> Fixed. Now, we declare domain level in config.yaml with the directive domain_level <blockquote type="cite"> <blockquote type="cite"> 4) Did you resolve the pytest fixtures which specifies which tests can be run under which domain level? </blockquote> </blockquote> In fact, we do not seem to have any tests yet that would require it. All the existing tests just use install_replica method, no matter how is it done. </blockquote> How about topology CI test? This can be executed only with domain level </blockquote> That's right. The topology test was updated. Patch is attached together with a proper version of 11-th patch (not a swap file, sorry about that). <blockquote type="cite">1, right? <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite"> 5) + '--ip-address', client.ip, why this change to client install? </blockquote> </blockquote> Right, it found to be unnecessary. <blockquote type="cite"> <blockquote type="cite"> Martin^2 </blockquote> 6) ************* Module ipatests.test_integration.tasks ipatests/test_integration/tasks.py:85: [E1123(unexpected-keyword-arg), allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in function call) </blockquote> Fixed <blockquote type="cite"> </blockquote> </blockquote> </blockquote> </blockquote> 1) + if not host.config.domain_level == None: + args.append("--domain-level=%i" % host.config.domain_level) always use: variable *is not None* 2) Why there is specified level 1 as default? IIRC we agreed that the default level is the one which is default in tested package. These should be None and "": + "domain_level": "1" + "DOMAINLVL": "1", 3) However, as I read the patch 12, and I'm right, the pytest.fixture needs to know the value of domain level before we can do any dynamic detection on master. So we should use the constants MAX_DOMAIN_LEVEL as default, for 2) </blockquote> Done <blockquote type="cite"> Also I'm not sure if the values are inherited from the DEFAULT_OUTPUT_DICT to code, I think it is not, so for this part you need default value, or the fixture will not work as expected. + self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL) </blockquote> This won't work in cases when domainlevel is explicitly set to 0 in config.yaml. This default value will always override the explicit one. </blockquote> wat? in case that kwargs is dict containing values from config file: In [1]: kwargs = dict(domain_level=0) In [2]: kwargs.get('domain_level', 123) Out[2]: 0 </blockquote> Respectively you should use this: <meta http-equiv="content-type" content="text/html; charset=windows-1252"> <pre style="background-color:#ffffff;color:#000000;font-family:'DejaVu Sans Mono';font-size:9.0pt;">self.domain_level = kwargs.get('domain_level') or MAX_DOMAIN_LEVEL because kwargs IMO contains undefined config values as None </pre> <blockquote cite="mid:562F424D.3060003@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite"> freeipa-tests depends on freeipa-python so the constants should be available in tests. So then you also need update this line + if not host.config.domain_level != MAX_DOMAIN_LEVEL: + args.append("--domain-level=%i" % host.config.domain_level) </blockquote> This would not work if domainlevel is not set in config.yaml, in which case the host.config.domain_level is None. </blockquote> Domain level will never be None because self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL) <blockquote type="cite"> <blockquote type="cite"> 4) Please add comment to function +def domainlevel(host): that it is useful for test where domain level will be raised dynamically, otherwise it may be lost after test refactoring as somebody may consider it as unneeded and replace it with config dict. </blockquote> Done <blockquote type="cite"> So summary is the 1) and 2) are replaced by 3) :) Martin^2 </blockquote> </blockquote> </blockquote> </body> </html>