[Freeipa-devel] [PATCH] ca-less tests updated
Oleg Fayans
ofayans at redhat.com
Fri Sep 9 13:22:06 UTC 2016
Hi David, team
According to your suggestions I've splitted my commits so that each
commit addresses some particular problem. One patch (0071) still
contains several unrelated fixes, but they mostly reflect changes in
error messages and really small but numerous bugfixes that I did not
consider worthy of a separate commit each. Please, whenever you have a
free time take a look at this new bunch of patches.
Thanks!
On 09/06/2016 04:41 PM, David Kupka wrote:
> 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
>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>
>
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0041.1-Fixed-method-failures-during-second-call-for-the-method.patch
Type: text/x-patch
Size: 1388 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0062-Added-basic-constraints-extension-to-the-CA-certs.patch
Type: text/x-patch
Size: 1129 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0063-Added-generation-of-missing-certs.patch
Type: text/x-patch
Size: 1152 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0064-Updated-ipa-server-installation-stdin-text.patch
Type: text/x-patch
Size: 1337 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0065-Create-a-method-that-cleans-all-ipa-certs.patch
Type: text/x-patch
Size: 1671 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0066-Added-teardown-methods-for-server-and-replica-instal.patch
Type: text/x-patch
Size: 2049 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0067-Removed-call-for-install-method-from-parent-class.patch
Type: text/x-patch
Size: 1151 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0068-Adapted-installation-methods-to-utilize-tasks.patch
Type: text/x-patch
Size: 10325 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0069-Fixed-incorrect-assert-in-verify_installation.patch
Type: text/x-patch
Size: 1475 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0070-Applied-correct-install-and-teardown-methods.patch
Type: text/x-patch
Size: 18279 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0071-Fixed-test-errors-in-calss-tests.patch
Type: text/x-patch
Size: 17796 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0072-Removed-outdated-command-options-test.patch
Type: text/x-patch
Size: 1722 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0073-Added-necessary-getkeytabs-calls-to-fixtures.patch
Type: text/x-patch
Size: 1680 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0074-Added-necessary-xfails.patch
Type: text/x-patch
Size: 4999 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0075-Updated-master-and-replica-installation-methods.patch
Type: text/x-patch
Size: 6383 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0076-Made-unapply_fixes-call-optional-at-master-uninstall.patch
Type: text/x-patch
Size: 1535 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0077-Enabled-negative-testing-for-cleaning-replication-agreements.patch
Type: text/x-patch
Size: 1323 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160909/2c5c5a01/attachment-0016.bin>
More information about the Freeipa-devel
mailing list