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

[David Kupka]

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
> +

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)


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)


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'])

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])
>

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
>

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"


-- 
David Kupka