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

Mon Nov 2 10:54:57 UTC 2015

Hi Martin,

On 11/02/2015 10:39 AM, Martin Basti wrote:
>
>
> 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
Fixed
>>>
>>> 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
Fixed
>>>
>>> 4)
>>> during testing this patch I used test_simple_replication and I found
>>> 4 bugs:
>> 3 bugs -----------------^^^
>>> #5419, #5420, #5421
Bug #5419 fixed, see patch N 15

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

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0011.5-replica-promotion-changes-in-tests.patch
Type: text/x-patch
Size: 6486 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151102/846f7af3/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0015-Fixed-A-record-creation-bug.patch
Type: text/x-patch
Size: 1149 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151102/846f7af3/attachment-0001.bin>