[Freeipa-devel] [PATCH] ca-less tests updated
Oleg Fayans
ofayans at redhat.com
Wed Sep 21 14:21:06 UTC 2016
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-0076.2-Made-unapply_fixes-call-optional-at-master-uninstall.patch
Type: text/x-patch
Size: 1552 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160921/b535d8e5/attachment.bin>
More information about the Freeipa-devel
mailing list