[Freeipa-devel] [PATCH] ca-less tests updated
Oleg Fayans
ofayans at redhat.com
Tue May 10 15:08:01 UTC 2016
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0013.1-Updated-the-script-creating-test-certificate-chains.patch
Type: text/x-patch
Size: 2642 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160510/7cad5702/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0014.6-Fixed-numerous-errors-in-ca-less-tests.patch
Type: text/x-patch
Size: 47033 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160510/7cad5702/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0039-Updated-generic-installation-methods-for-ca-less-tests.patch
Type: text/x-patch
Size: 6971 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160510/7cad5702/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0040-Added-cleaning-of-etc-httpd-alias-upon-server-uninstall.patch
Type: text/x-patch
Size: 1308 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160510/7cad5702/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0041-Fixed-method-failures-during-second-call-for-the-method.patch
Type: text/x-patch
Size: 1223 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160510/7cad5702/attachment-0004.bin>
More information about the Freeipa-devel
mailing list