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

[Oleg Fayans]

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.

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