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

[Martin Basti]

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.

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