[Freeipa-devel] [PATCH] ca-less tests updated
David Kupka
dkupka at redhat.com
Mon Sep 12 07:51:01 UTC 2016
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
>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
>
--
David Kupka
More information about the Freeipa-devel
mailing list