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

Tue Oct 27 11:06:12 UTC 2015

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

>
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0011.3-replica-promotion-changes-in-tests.patch
Type: text/x-patch
Size: 6021 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151027/834bd959/attachment.bin>