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
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
