Hi all,
Bump for the review of the 0013 patch. The script it addresses can be
reused in some WebUI tests - one more reason to have it reviewed/merged
The rest patches should be re-tested, since they were prepared a good
while ago
On 05/10/2016 05:08 PM, Oleg Fayans wrote:
Hi David,
After quite a while and some more struggles here comes the updated
version of the patch together with other patches fixing things in
ipatests/test_integration/tasks.py
Server and replica installation was refactored in a way to utilize the
code from tasks.py as much as it is possible
The full set of necessary patches is attached
On 04/20/2016 10:35 AM, David Kupka wrote:
On 19/04/16 11:13, Oleg Fayans wrote:
OK, that one, though passing lint, did not actually work. I gave up my
attempts to define method decorators inside the class. Now it passes
lint AND works:)
Hi Oleg!
1) Current commit message is useless. Please use it to describe what is
the point of the patch.
2) $ git show -U0 | pep8 --diff
./ipatests/test_integration/test_caless.py:66:1: E302 expected 2 blank
lines, found 1
./ipatests/test_integration/test_caless.py:74:1: E302 expected 2 blank
lines, found 1
./ipatests/test_integration/test_caless.py:820:5: E303 too many blank
lines (2)
./ipatests/test_integration/test_caless.py:825:80: E501 line too long
(80 > 79 characters)
./ipatests/test_integration/test_caless.py:1035:44: E225 missing
whitespace around operator
3) Isn't there a way to do this with pytest's fixtures?
+def server_install_teardown(func):
+ def wrapped(*args):
+ try:
+ func(*args)
+ finally:
+ args[0].uninstall_server()
+ return wrapped
+
+def replica_install_teardown(func):
+ def wrapped(*args):
+ try:
+ func(*args)
+ finally:
+ # Uninstall replica
+ replica = args[0].replicas[0]
+ tasks.kinit_admin(args[0].master)
+ args[0].uninstall_server(replica)
+ args[0].master.run_command(['ipa-replica-manage', 'del',
+ replica.hostname, '--force'],
+ raiseonerr=False)
+ args[0].master.run_command(['ipa', 'host-del',
+ replica.hostname],
+ raiseonerr=False)
+ return wrapped
+
There is a standard pytest method called 'method_teardown', that is
indent to be executed after each test method, but with our setup it does
not work.
4) Is it necessary to create the $TEST_DIR in the test? Isn't it created
by the framework?
+ host.transport.mkdir_recursive(host.config.test_dir)
Removed.
5) I don't think the comment match the code.
+ # Remove CA cert in /etc/pki/nssdb, in case of failed
(un)install
+ for host in cls.get_all_hosts():
+ cls.uninstall_server(host)
+
super(CALessBase, cls).uninstall(mh)
Not actual anymore
6) No! Create list with one element, iterate that list and append every
item to the other list. Maybe there's better way (Hint: append).
I've seen this on multiple places.
if unattended:
args.extend(['-U'])
Agreed
7) Why don't you (extend and) use
ipatests.test_integaration.tasks.(un)install_{master,replica}?
This could be done pretty much all over the code.
host.run_command(['ipa-server-install', '--uninstall', '-U'])
8) Use ipaplatform.paths for certutil and other binaries. If the binary
is not there feel free to add it.
I've seen this on multiple places.
+ host.run_command(['certutil', '-d', paths.NSS_DB_DIR, '-D',
+ '-n', 'External CA cert'],
+ raiseonerr=False)
+ # A workaround forhttps://fedorahosted.org/freeipa/ticket/4639
+ result = host.run_command(['certutil', '-L', '-d',
+ paths.HTTPD_ALIAS_DIR])
+ for rawcert in result.stdout_text.split('\n')[4: -1]:
+ cert = rawcert.split(' ')[0]
+ host.run_command(['certutil', '-D', '-d',
paths.HTTPD_ALIAS_DIR,
+ '-n', cert])
Done
9) certmonger is system service. You can check if is is .enabled() and
.running(). And IIUC the comment is negation of what the code does.
# Verify certmonger was not started
result = host.run_command(['getcert', 'list'],
raiseonerr=False)
- assert result > 0
- assert ('Please verify that the certmonger service has
been '
- 'started.' in result.stdout_text),
result.stdout_text
+ assert result.returncode == 0
10) What is the point of calling uninstall_server() when it will be
called in the finally block of server_install_teardown anyway?
+ @server_install_teardown
def test_revoked_http(self):
"IPA server install with revoked HTTP certificate"
if result.returncode == 0:
+ self.uninstall_server()
raise nose.SkipTest(
"Known CA-less installation defect, see "
+"https://fedorahosted.org/freeipa/ticket/4270";)
assert result.returncode > 0
Removed
Nitpick) Do not mix fixing typos/grammar/spelling/style with functional
changes.
- def test_incorect_http_pin(self):
+ @pytest.mark.xfail(reason='freeipa ticket 5378')
+ def test_incorrect_http_pin(self):
"Install new HTTP certificate with incorrect PKCS#12 password"
Removed
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
--
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
