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

Tue Oct 27 12:56:58 UTC 2015

On 10/27/2015 01:22 PM, Martin Basti wrote:
>
>
> 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)

I do not get this. If we do not specify domain_level in config.yaml, it 
will automatically be populated with a MAX_DOMAIN_LEVEL value. That 
means domain_level will never ever possibly be None.

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

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.