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

Tue Oct 27 12:58:59 UTC 2015


On 27.10.2015 13:56, Oleg Fayans wrote:
>
>
> 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.
I do not know how pytest works inside, if you are 100% sure, that the 
case written above cannot happen, I'm fine with it.
Martin
>
>>
>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>