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

[Martin Basti]

On 27.10.2015 10:22, Martin Basti wrote:
>
>
> 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
>
Respectively you should use this:

self.domain_level = kwargs.get('domain_level')or MAX_DOMAIN_LEVEL

because kwargs IMO contains undefined config values as None



>>
>>>
>>> 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
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151027/9942d829/attachment.htm>