[Freeipa-devel] [TEST][Patch-0030]Next part of replica promotion tests

[Martin Basti]

On 05.04.2016 18:15, Martin Babinsky wrote:
> On 04/01/2016 11:41 AM, Oleg Fayans wrote:
>> Hi Martin,
>>
>> Thanks for the review! The new version is attached
>>
>> On 03/24/2016 06:08 PM, Martin Babinsky wrote:
>>> On 03/21/2016 01:51 PM, Oleg Fayans wrote:
>>>>
>>>>
>>>>
>>> Hi Oleg,
>>>
>>> I have a few comments:
>>>
>>> 1.)
>>> please make the commit message more clear, briefly describe what 
>>> kind of
>>> test cases were added to the suite and maybe add a link to the test 
>>> plan.
>>
>> Done
>>
>>>
>>> 2.)
>>> I see negative test scenarios for attempting to issue
>>> 'ipa-csreplica-manage connect' and 'disconnect' under domain level 1.
>>> However, for full coverage there should be also a negative test case 
>>> for
>>> 'ipa-csreplica-manage del' which should also issue error in domain 
>>> level
>>> 1, see
>>> https://git.fedorahosted.org/cgit/freeipa.git/commit/install/tools/ipa-csreplica-manage?h=ipa-4-3&id=6119dbb9a915283434f718b38a70017e3ad00840 
>>>
>>>
>>>
>>> Could you please add this case to the patch and also to the Test 
>>> plan so
>>> that we have full coverage of this?
>>
>> Done
>>
>>>
>>> 3.)
>>> test_one_command_installation exploded during client enrollment part on
>>> "Joining realm failed: incorrect password". This is probably caused by
>>> missing '-P', 'admin' option here:
>>> """
>>> +        self.replicas[0].run_command(['ipa-replica-install', '-p',
>>> + self.master.config.admin_password,
>>> +                                     '-n', self.master.domain.name,
>>> +                                     '-r', self.master.domain.realm])
>>> +
>>> """
>>
>> Fixed. Turned out, it's enough to just provide '-w'
>>
>>>
>>> 4.)
>>> I am not very happy about the organization of
>>> 'TestUnprivilegedUserPermissions' class.
>>>
>>> For starters, I would add this whole block:
>>> """
>>> +        password = self.master.config.dirman_password
>>> +        new_password = '$ome0therPaaS'
>>> +        replica = self.replicas[0]
>>> +        adduser_stdin_text = "%s\n%s\n" %
>>> (self.master.config.admin_password,
>>> + self.master.config.admin_password)
>>> +        user_kinit_stdin_text = "%s\n%s\n%s\n" % (password, 
>>> new_password,
>>> + new_password)
>>> +        tasks.kinit_admin(self.master)
>>> +        self.master.run_command(['ipa', 'user-add', 'testuser',
>>> '--password',
>>> +                                 '--first', 'John', '--last', 'Donn'],
>>> + stdin_text=adduser_stdin_text)
>>> +        # Now we need to change the password for the user
>>> +        self.master.run_command(['kinit', 'testuser'],
>>> + stdin_text=user_kinit_stdin_text)
>>> +        # And again kinit admin
>>> +        tasks.kinit_admin(self.master)
>>> """
>>>
>>> into 'install()' method, since it indeed sets-up the test harness. You
>>> can add the user name and password to class members so that you can 
>>> then
>>> use them from the test cases. Which brings me to the second point: I
>>> know that the test plan mentions this as a single test case, but I 
>>> would
>>> like this:
>>>
>>> """
>>> +        result1 = replica.run_command(['ipa-client-install', '-p',
>>> 'testuser',
>>> +                                       '-w', new_password,
>>> +                                       '--domain', 
>>> replica.domain.name,
>>> +                                       '--realm', 
>>> replica.domain.realm,
>>> '-U'],
>>> +                                      raiseonerr=False)
>>> +        assert_error(result1, "No permission to join this host", 1)
>>> +        tasks.install_client(self.master, replica)
>>> +        result2 = replica.run_command(['ipa-replica-install', '-P',
>>> 'testuser',
>>> +                                       '-p', new_password,
>>> +                                       '-n', self.master.domain.name,
>>> +                                       '-r', 
>>> self.master.domain.realm],
>>> +                                      raiseonerr=False)
>>> +        assert_error(result2,
>>> +                     "Insufficient privileges to promote the 
>>> server", 1)
>>> +        self.master.run_command(['ipa', 'group-add-member', 'admins',
>>> +                                 '--users=testuser'])
>>> +
>>> +        replica.run_command(['ipa-replica-install', '-P', 'testuser',
>>> +                             '-p', new_password,
>>> +                             '-n', self.master.domain.name,
>>> +                             '-r', self.master.domain.realm])
>>> """
>>>
>>> to be split into three separate test methods for the sake of 
>>> clarity, e.g.:
>>> "test_client_enrollment_by_unprivileged_user"
>>> "test_replica_install_by_unprovileged_user"
>>> "test_replica_install_after_adding_to_admin_group"
>>
>> I like that! Implemented.
>>
>>>
>>> 5.)
>>> """
>>> +        result = self.replicas[0].run_command(['ipa-server-install',
>>> +                                               '--uninstall', '-U'],
>>> + raiseonerr=False)
>>> +        assert("Uninstallation leads to disconnected topology"
>>> +               in result.stderr_text)
>>> +        self.replicas[0].run_command(['ipa-server-install', 
>>> '--uninstall',
>>> +                                      '-U',
>>> '--ignore-topology-disconnect'])
>>> """
>>> here you should assert against command stdout, since the error message
>>> is emitted only by plain print(). Yes it is weird but that's the way it
>>> is. It will probably be changed when I implement
>>> https://fedorahosted.org/freeipa/ticket/5588 so we can fix it when the
>>> ticket is finished.
>>
>> Yeah, I've noticed that already. Fixed
>>
>>>
>>
>
> Thanks,
>
> ACK, the code looks good and tests are shining green.
>
Pushed to:
master: ab3b4a92a85b550429429acc238f2e90128de565
ipa-4-3: fb8e97b06db8fbca517fb9c91bc5c28368b3da56