[Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 26.10.2015 08:59, Oleg Fayans wrote:
>
>
> On 10/23/2015 03:10 PM, Martin Basti wrote:
>>
>>
>> On 23.10.2015 15:00, Oleg Fayans wrote:
>>> Hi Martin,
>>>
>>> Here comes the updated version.
>>>
>>> On 10/22/2015 05:38 PM, Martin Basti wrote:
>>>>
>>>>
>>>> On 22.10.2015 15:23, Martin Basti wrote:
>>>>>
>>>>> On 22.10.2015 14:13, Oleg Fayans wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>> thank you for the patch.
>>>>>
>>>>> 1)
>>>>> please remove the added empty lines, they are unrelated to this 
>>>>> ticket
>>>
>>> done
>>>
>>>>>
>>>>> 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)
>>>
>>> done
>>>
>>>>>
>>>>> 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
>>>
>>> Fixed. Now, we declare domain level in config.yaml with the directive
>>> domain_level
>>>
>>>>>
>>>>> 4)
>>>>> Did you resolve the pytest fixtures which specifies which tests 
>>>>> can be
>>>>> run under which domain level?
>>>
>>> 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.
>> How about topology CI test? This can be executed only with domain level
>
> 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).
>
>> 1, right?
>>>
>>>>>
>>>>> 5)
>>>>> +                        '--ip-address', client.ip,
>>>>>
>>>>> why this change to client install?
>>>
>>> Right, it found to be unnecessary.
>>>
>>>>>
>>>>> Martin^2
>>>>>
>>>>>
>>>> 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)
>>>
>>> Fixed
>>>
>>>>
>>>>
>>>
>>
>

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)

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)

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)

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.

So summary is the 1) and 2) are replaced by 3) :)

Martin^2