[Freeipa-devel] [PATCH] First part of the replica promotion tests + testplan

Fri Nov 27 14:04:22 UTC 2015

Hi Martin,

All your suggestions were taken into account. Both patches are updated. 
Thank you for your help!

On 11/26/2015 10:50 AM, Martin Basti wrote:
>
>
> On 26.11.2015 10:04, Oleg Fayans wrote:
>> Hi Martin,
>>
>> I agree to all your points but one. please, see my comment below
>>
>> On 11/25/2015 07:42 PM, Martin Basti wrote:
>>> Hi,
>>>
>>> 0) Note
>>> Please be aware of https://fedorahosted.org/freeipa/ticket/5469 during
>>> KRA testing
>>>
>>> 1)
>>> Please do not use MIN and MAX_DOMAIN_LEVEL constants, this may change
>>> over time, use DOMAIN_LEVEL_0 and DOMAIN_LEVEL_1 for domain level 0
>>> and 1
>>>
>>> 2)
>>> Why uninstall KRA then server, is not enough just uninstall server which
>>> covers KRA uninstall?
>>>
>>> +    def teardown_method(self, method):
>>> +        for host in self.replicas:
>>> +            host.run_command(self.kra_uninstall, raiseonerr=False)
>>> +            tasks.uninstall_master(host)
>>>
>>>
>>> 3)
>>> Can be this function more generic? It should allow specify host where
>>> KRA should be installed not just master
>>>
>>> +    def test_kra_install_master(self):
>>> +        self.master.run_command(self.kra_install)
>>>
>>>
>>> 4)
>>>
>>> TestLevel0(Dummy):
>>> Can be the test name more specific, something like
>>> TestReplicaPromotionLevel0
>>>
>>>
>>> 5)
>>> please remove this, the patch is on review and it will be pushed sooner
>>> than tests
>>> +    @pytest.mark.xfail  # Ticket N 5455
>>>
>>> and as I mentioned in ticket #5455, I cannot reproduce it with
>>> ipa-kra-install, so please provide steps to reproduce if you insist that
>>> this still does not work as expected with KRA.
>>>
>>> 6) This is completely wrong, it removes everything that we tried to
>>> achieve with previous patches with domain level in CI
>>
>> Actually, being able to configure domain level per class is WAY more
>> convenient, than to always have to think which domain level is
>> appropriate for which particular test during jenkins job
>> configuration. In fact, I should have thought about it from the very
>> beginning. For example, in test_replica_promotion.py we have on class,
>> which intiates with domain level = 1, while others - with domain level
>> 0. With config-based approach, we would have to implement a separate
>> step that raises domain level. Overall, I am against the approach,
>> when you have to remember to set certain domain level in config for
>> any particular test. The tests themselves should be aware of the
>> domain level they need.
> I do not say that we should not have something that overrides settings
> in from config in a particular test case, I say your patch is doing it
> wrong.
>
> I agree it is useful to have param domain_level in install_master, and
> intall_topo methods,  but is cannot be MAX_DOMAIN_LEVEL by default,
> because with your current patch the domain_level in config is not used
> at all, it will be always MAX_DOMAIN_LEVEL
>
> For example I want to achieve this goal:
> test_vault.py, this test suite can run on domain level1 and on domain
> level0, so with one test we can test 2 domain levels just with putting
> domain level into config file.
>
> I agree that with extraordinary test like replica promotion test is, we
> need something that allows override the config file.
>
> As I said bellow, domain_level default value should be None in
> install_master and install_topo plugin. If domain level was specified
> use the specified one, if not (value is None) use the domain level from
> config file.

Agreed :)

>
> Martin
>>
>>>
>>> [PATCH] Enabled setting domain_level per class derived from
>>> TestIntegration
>>>
>>> When I configure domain level 0 in yaml config, how is this supposed to
>>> get into install methods when you removed that code?
>>>
>>> -        "--domain-level=%i" % host.config.domain_level
>>> +        "--domain-level=%i" % domain_level
>>>
>>>
>>> You always use MAX_DOMAIN_LEVEL in this case or whatever is specified in
>>> domain_level option.
>>> I suggest to use domain_level=None, and when it is None use
>>> 'host.config.domain_level', if it is not None, use 'domain_level'
>>>
>>> With this we can specify domain level in config file for test that can
>>> be used for both domain levels and you can manually specify domain level
>>> for test that requires specific domain level.
>>>
>>> Also this should go away
>>>
>>>       @classmethod
>>>       def install(cls, mh):
>>> +        if hasattr(cls, "domain_level") and cls.master:
>>> +            cls.master.config.domain_level = cls.domain_level
>>>           if cls.topology is None:
>>>               return
>>>
>>> I do not see reason why test should override configuration in config in
>>> this case.
>>>
>>> Martin
>>>
>>> On 25.11.2015 16:44, Oleg Fayans wrote:
>>>> Hi,
>>>>
>>>> Here is the updated version of the patch (more tests + fixed the
>>>> issues of the first one) + patch 0017, that implements the necessary
>>>> changes in the background code, i. e. patch 16 does not work without
>>>> patch 17
>>>>
>>>> On 11/18/2015 05:20 PM, Martin Basti wrote:
>>>>>
>>>>>
>>>>> On 09.11.2015 15:09, Oleg Fayans wrote:
>>>>>> Hi guys,
>>>>>>
>>>>>> Here are first two automated testcases from this (so far incomplete)
>>>>>> testplan: http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
>>>>>>
>>>>>> Testplan review is highly appreciated
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>> PATCH 16: NACK
>>>>>
>>>>> 1)
>>>>> What is the reason to add an unused parameter to 'domain_level' to
>>>>> install_topo()?
>>>>> Also it is good practise to add new option as the last parameter.
>>>>>
>>>>> 2)
>>>>> cab you in both tests specify a domain level with constant instead of
>>>>> number literal?
>>>>>
>>>>> 3)
>>>>> both test call install_topo with custom domain level, but it cannot
>>>>> work
>>>>> because 1)  (did you run the test?)
>>>>>
>>>>> 4)
>>>>> How the test "TestLevel1" is supposed to work?
>>>>> Respectively why there is call of install_topo() that installs
>>>>> replica.
>>>>> As this test just tests that ipa-replica-prepare is not working
>>>>> anymore,
>>>>> is it worth to spend 20 minutes with installing replica and then
>>>>> just no
>>>>> tot use it? IMO to install master in install step is enough.
>>>>>
>>>>> Martin^2
>>>>>
>>>>
>>>
>>
>

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0016.2-First-part-of-replica-promotion-tests.patch
Type: text/x-patch
Size: 10017 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151127/d6fdbed1/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0017.1-Enabled-setting-domain_level-per-class.patch
Type: text/x-patch
Size: 4941 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151127/d6fdbed1/attachment-0001.bin>