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

[Martin Basti]

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