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

Tue Oct 27 12:22:21 UTC 2015


On 27.10.2015 12:06, Oleg Fayans wrote:
> Hi Martin,
>
> On 10/27/2015 10:50 AM, Martin Basti wrote:
>>
>>
>> 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
>
> Yep, right you are, it works as expected. Now the line looks like:
> self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)
>
>>>
>> Respectively you should use this:
>>
>> self.domain_level = kwargs.get('domain_level')or MAX_DOMAIN_LEVEL
> Now that would definitely not work in case of domain_level is 0:
> 0 or 1 is always 1
Oh right.

I had discussion with Tomas, and we should add there check if it is not 
None, in case that kwargs contains {'domain_level': None} (One does not 
know with test when the None value appears there)

self.domain_level = kwargs.get('domain_level')
if self.domain_level is None:
     self.domain_level = MAX_DOMAIN_LEVEL

As we cannot user 'or' expression with integers, so this is the right way.
>
>>
>> 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
>>>>
>>>
>>
>