On 20/04/16 10:35, 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

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

7) Why don't you (extend and) use
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',
+                              '-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'],
-            assert result > 0
-            assert ('Please verify that the certmonger service has
been '
-                    'started.' in 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 "

          assert result.returncode > 0

Nitpick) Do not mix fixing typos/grammar/spelling/style with functional

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

I forget to mention the most important problem:

0) It does not work.
============================= test session starts ==============================
platform linux2 -- Python 2.7.11 -- py-1.4.30 -- pytest-2.7.3
rootdir: /usr/lib/python2.7/site-packages/ipatests, inifile: pytest.ini
plugins: multihost, sourceorder
collected 88 items


David Kupka

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to