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

[Martin Babinsky]

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.

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?

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])
+
"""

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"

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.

-- 
Martin^3 Babinsky