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

Thu Oct 29 17:32:58 UTC 2015


On 29.10.2015 18:31, Martin Basti wrote:
> NACK
>
> 1)
> DO NOT use tabs in code to indent
>
> 2)
> Replica uninstallation does not work, uninstallation works different 
> with domain level 0 and 1 (currently uninstallation with domain 1 
> level will not work, it is known issue, but still the patch should 
> solve the uninstallation)
>
> 3)
> apply_common_fixes(host)
> Method for domain_level 1 is called twice, first time in replica 
> install, second time in client install
>
> 4)
> during testing this patch I used test_simple_replication and I found 4 
> bugs:
3 bugs -----------------^^^
> #5419, #5420, #5421
> IMO it is related only to this one test case and to pass this test 
> case #5419 or #5421 must be fixed.
>
>
> On 27.10.2015 16:34, Oleg Fayans wrote:
>> Hi Martin,
>>
>> The updated patch is attached
>>
>> On 10/27/2015 01:58 PM, Martin Basti wrote:
>>>
>>>
>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>