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

Tue Oct 27 15:34:11 UTC 2015

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

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0011.4-replica-promotion-changes-in-tests.patch
Type: text/x-patch
Size: 6171 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151027/9e0621e2/attachment.bin>