[Freeipa-devel] [PATCH] ca-less tests updated
Oleg Fayans
ofayans at redhat.com
Thu Sep 22 10:55:41 UTC 2016
Fixed patch N 41
On 09/21/2016 04:21 PM, Oleg Fayans wrote:
> Patch-0076 rebased to current master
>
> On 09/21/2016 02:41 PM, Oleg Fayans wrote:
>> 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.3-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/20160922/8dd67da5/attachment.bin>
More information about the Freeipa-devel
mailing list