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

[Oleg Fayans]

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

> 

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0030.1-Added-5-more-tests-to-Replica-Promotion-testsuite.patch
Type: text/x-patch
Size: 8897 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160401/85441b31/attachment.bin>