[Freeipa-devel] [PATCH] ca-less tests updated

[Oleg Fayans]

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.