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:

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.


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

Could you please add this case to the patch and also to the Test plan so
that we have full coverage of this?


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'

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)
+        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',
+                                 '--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',
+                                       '-w', new_password,
+                                       '--domain', replica.domain.name,
+                                       '--realm', replica.domain.realm,
+                                      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',
+                                       '-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.:

I like that! Implemented.

+        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',
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


ACK, the code looks good and tests are shining green.

Martin^3 Babinsky

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to