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

[Oleg Fayans]

Hi Martin,

The updated version of the patch is attached. Please, see my comments below

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.

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

>
> 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.2-replica-promotion-changes-in-tests.patch
Type: text/x-patch
Size: 5746 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151027/a8e000be/attachment.bin>