[Freeipa-devel] [PATCH] ca-less tests updated
Oleg Fayans
ofayans at redhat.com
Wed Sep 21 12:41:58 UTC 2016
Hi David,
As per your comments the patches were once again refactored. I am
attaching the full set of them, please ignore any previous versions
The patches apply cleanly on master and pylint swallows the resulting
code silently
On 09/12/2016 09:51 AM, David Kupka wrote:
> Hi Oleg,
> thank you, now it's completely different game.
> Please add prefix to commit message summaries. Simply prepending "tests:
> " should be OK.
>
> 0041 - -h is deprecated in favor of -H.
> 0062 - 0068 - LGTM
> 0069 - I see 2 unrelated changes in the patch, please split them:
> - 1 - certutil - > paths.CERTUTIL
> - 2 - assert
> 0070 - I see 2 unrelated changes in the patch, please split them:
> - 1 - teardown
> - 2 - TestReplicaInstall.setUp -> TestReplicaInstall.install
> 0071 - typos in commit message, I see 5 unrelated changes in that patch:
> - 1 - error messages in assert
> - 2 - certificates used
> - 3 - verify_installation called only in DOMAIN_LEVEL_0.
> - 4 - TestCertinstall.install
> - 5 - TestCertinstall.certinstall
> 0072 - 0077 - LGTM
>
> On 09/09/16 15:22, Oleg Fayans wrote:
>> 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.2-Fixed-method-failures-during-second-call-for-the-method.patch
Type: text/x-patch
Size: 1394 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/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: 1136 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/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: 1159 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/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: 1344 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/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: 1678 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/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: 2056 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/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: 1158 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/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: 10332 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0069.1-Fixed-incorrect-assert-in-verify_installation.patch
Type: text/x-patch
Size: 1112 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0070.1-Applied-correct-teardown-methods.patch
Type: text/x-patch
Size: 17742 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0072-Removed-outdated-command-options-test.patch
Type: text/x-patch
Size: 1729 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0010.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: 1687 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0074.1-Added-necessary-xfails.patch
Type: text/x-patch
Size: 4940 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0012.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: 6390 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0076.1-Made-unapply_fixes-call-optional-at-master-uninstall.patch
Type: text/x-patch
Size: 1553 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0014.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: 1330 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0078-Replaced-hardcoded-certutil-with-imported-from-paths.patch
Type: text/x-patch
Size: 990 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0079-test-Replaced-unused-setUp-method-with-install.patch
Type: text/x-patch
Size: 1474 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0017.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0080-test-fixed-expects-of-incorrect-error-messages.patch
Type: text/x-patch
Size: 7557 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0018.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0081-test-Fixed-Usage-of-improper-certs-in-ca-less-tests.patch
Type: text/x-patch
Size: 3467 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0019.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0082-tests-Implemented-check-for-domainlevel.patch
Type: text/x-patch
Size: 5390 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0020.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0083-tests-Standardized-replica_preparation-in-test_no_ce.patch
Type: text/x-patch
Size: 1344 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0021.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0084-tests-added-verbose-assert-to-test_service_disable.patch
Type: text/x-patch
Size: 1427 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0022.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0085-tests-fixed-super-method-invocation.patch
Type: text/x-patch
Size: 927 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0023.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0086-tests-fixed-certinstall-method.patch
Type: text/x-patch
Size: 1265 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0024.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0087-Reverted-erronous-asserts-in-4-tests.patch
Type: text/x-patch
Size: 2277 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0025.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0088-Fixed-code-styling-to-make-pep8-happy.patch
Type: text/x-patch
Size: 3287 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/a69222c5/attachment-0026.bin>
More information about the Freeipa-devel
mailing list