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

[Martin Basti]

On 27.11.2015 15:04, Oleg Fayans wrote:
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>
./make-lint
************* Module ipatests.test_integration.base
ipatests/test_integration/base.py:66: [E1101(no-member), 
IntegrationTest.install] Class 'IntegrationTest' has no 'domain_level' 
member)
************* Module ipatests.test_integration.test_replica_promotion
ipatests/test_integration/test_replica_promotion.py:16: 
[E1101(no-member), Dummy.install] Class 'Dummy' has no 'domain_level' 
member)
ipatests/test_integration/test_replica_promotion.py:117: 
[E1101(no-member), TestCAInstall.test_ca_install_without_replica_file] 
Module 'ipatests.test_integration.tasks' has no 'setup_replica' member)


Is it so hard to run pylint before patch is posted for review?