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

[Martin Basti]

On 27.10.2015 10:00, Oleg Fayans wrote:
> Hi Martin,
>
> The updated version of the patch is attached. Please, see my comments 
> below
My comments inline, I may be completely wrong in how the test suite 
work, so feel free to correct me.

Martin

>
> On 10/26/2015 06:48 PM, Martin Basti wrote:
>>
>>
>> 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)
> Done
>>
>> 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)
>
> 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.
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

>
>>
>> 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)
>
> This would not work if domainlevel is not set in config.yaml, in which 
> case the host.config.domain_level is None.
Domain level will never be None because self.domain_level = 
kwargs.get('domain_level', MAX_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.
> Done
>
>>
>> So summary is the 1) and 2) are replaced by 3) :)
>>
>> Martin^2
>