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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky