[Freeipa-devel] [PATCH] ca-less tests updated
David Kupka
dkupka at redhat.com
Tue Sep 6 14:41:30 UTC 2016
Hi Oleg!
0013 - It looks like there are two unrelated changes, addition of CRL
distribution extension and creating certificate signed by no longer
existing CA. Please create separate patch for each of the changes, and
describe the change and reason for it in commit messages.
0014 - Could you please split the patch to "numerous" commit each fixing
one error? Please also describe each fix so everyone has at least vague
idea about the patch without reading its code. Also why do you introduce
global variable config, I don't see its used anywhere.
0039 - It looks like multiple different changes and commit message says
nothing again. Please split and describe what did you change and why.
0041 - Looks like weird workaround to me. It would be better to
investigate the root cause and fix it. Or at least describe the cause in
commit message and code comment if it can't be fixed. Also "-h is
deprecated in favor of -H" says man 1 ldapmodify.
On 05/09/16 14:32, Oleg Fayans wrote:
> Hi guys,
>
> Finally the ca-less tests are stable. Here in the attachment is the full
> set of necessary patches.
>
>
> On 08/09/2016 10:57 AM, Oleg Fayans wrote:
>> 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
>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>
--
David Kupka
More information about the Freeipa-devel
mailing list