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

[Martin Basti]

On 29.10.2015 18:32, Martin Basti wrote:
>
>
> 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)
This is not valid, my bad, I was confused with new behaviour of replica 
uninstallation, but it is bug not a feature.
So replica uninstallation is the same for level 0 and 1
Sorry.
>>
>> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>