[Freeipa-devel] [PATCH] First part of the replica promotion tests + testplan
Oleg Fayans
ofayans at redhat.com
Wed Dec 2 15:18:53 UTC 2015
Hi Martin,
On 12/01/2015 04:08 PM, Martin Basti wrote:
>
>
> On 27.11.2015 16:26, Oleg Fayans wrote:
>> And patch N 16 passes lint too:
>>
>> On 11/27/2015 04:03 PM, Oleg Fayans wrote:
>>> Hi,
>>>
>>> On 11/27/2015 03:26 PM, Martin Basti wrote:
>>>>
>>>>
>>>> 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?
>>>
>>> Sorry, my bad. Fixed.
>>>
>>>
>>>
>>
>
> 1)
> Why is this change in the patch?
> - # Clean up the test directory
> - host.run_command(['rm', '-rvf', host.config.test_dir])
Otherwise 2 out of 8 tests fail with IOError at line 78 of
ipatests/test_integration/tasks.py
I do not understand yet how does this happen, but if you remove ipatests
folder once, it then fails to be created again.
> -
>
>
> 2)
> is enough to have this check only in install_master, install_topo can
> pass None to install_master
> + if domain_level is None:
> + domain_level = master.config.domain_level
Done
>
>
> 3)
> IMO replica-manage del should cleanup hosts entry, so following code
> should not be needed.
> + if cleanhost:
> + kinit_admin(master)
> + master.run_command(["ipa", "host-del", "--updatedns",
> replica.hostname],
> + raiseonerr=False)
Well, in fact it does not. At least the corresponding dns record stays
and causes the subsequent ipa-client-install to fail. Probably it's a
bug. On the other hand, if I promote an existing client to replica and
then delete this replica, then, I probably want the host record (that
was created during client-install) to stay in the system. So, does not
look like a bug to me.
>
> 4)
> This variable is not used
> + kra_uninstall = ["ipa-kra-install", "--uninstall", "-U"]
Removed
>
> 5)
> Why do you need this
> + kra_install = ["ipa-kra-install", "-U", "-p", config.dirman_password]
> when you implemented tasks.install_kra that returns the exactly the same
> result?
Right. Removed
>
> 6)
> PEP8
> ./ipatests/test_integration/tasks.py:928:1: E302 expected 2 blank lines,
> found 1
> ./ipatests/test_integration/tasks.py:934:1: E302 expected 2 blank lines,
> found 1
> ./ipatests/test_integration/tasks.py:939:1: E302 expected 2 blank lines,
> found 1
> ./ipatests/test_integration/tasks.py:943:1: E302 expected 2 blank lines,
> found 1
> ./ipatests/test_integration/tasks.py:950:80: E501 line too long (80 > 79
> characters)
>
> ./ipatests/test_integration/test_replica_promotion.py:29:80: E501 line
> too long (85 > 79 characters)
> ./ipatests/test_integration/test_replica_promotion.py:64:80: E501 line
> too long (85 > 79 characters)
> ./ipatests/test_integration/test_replica_promotion.py:67:80: E501 line
> too long (88 > 79 characters)
> ./ipatests/test_integration/test_replica_promotion.py:93:80: E501 line
> too long (80 > 79 characters)
> ./ipatests/test_integration/test_replica_promotion.py:94:80: E501 line
> too long (83 > 79 characters)
> ./ipatests/test_integration/test_replica_promotion.py:118:80: E501 line
> too long (81 > 79 characters)
> ./ipatests/test_integration/test_replica_promotion.py:128:80: E501 line
> too long (80 > 79 characters)
> ./ipatests/test_integration/test_replica_promotion.py:129:80: E501 line
> too long (82 > 79 characters)
> ./ipatests/test_integration/test_replica_promotion.py:181:80: E501 line
> too long (80 > 79 characters)
Most of these complaints are unrelated to the current patches.
It's better to create a separate patch addressing PEP8 errors.
>
> 7)
> Why this must be stored in instance? IMO to have it stored as local
> variable is perfect
> TestKRAInstall, TestCAInstall
> self.replica1_filename = tasks.get_replica_filename(replica1)
> self.replica2_filename = tasks.get_replica_filename(replica2)
Agree. Fixed
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0016.4-First-part-of-replica-promotion-tests.patch
Type: text/x-patch
Size: 9837 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151202/5ed15e58/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0017.3-Enabled-setting-domain_level-per-class.patch
Type: text/x-patch
Size: 5136 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151202/5ed15e58/attachment-0001.bin>
More information about the Freeipa-devel
mailing list