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.
From 86e8f08f52c477902a698220454c96220f3b1461 Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Wed, 21 Sep 2016 16:19:57 +0200
Subject: [PATCH] tests: Made unapply_fixes call optional at master
 uninstallation

Unapply fixes removes the temporary testing folder at ~/ipatests, which
contains some artifacts like root.pem that need to be persistent between tests
in the test_caless testsuite. There has to be the way to skip the deletion of
this testfolder
---
 ipatests/test_integration/tasks.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 851358e98420d9b43b9784c0460903d1b06aa356..f22f4f1aae37a89fecb055db0d9213bbf5e82ac9 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -679,7 +679,7 @@ def kinit_admin(host, raiseonerr=True):
 
 
 def uninstall_master(host, ignore_topology_disconnect=True,
-                     ignore_last_of_role=True):
+                     ignore_last_of_role=True, clean=True):
     host.collect_log(paths.IPASERVER_UNINSTALL_LOG)
     uninstall_cmd = ['ipa-server-install', '--uninstall', '-U']
 
@@ -706,7 +706,8 @@ def uninstall_master(host, ignore_topology_disconnect=True,
                      "xargs rm -fv", raiseonerr=False)
     host.run_command("find /run/ipa -name 'krb5*' | xargs rm -fv",
                      raiseonerr=False)
-    unapply_fixes(host)
+    if clean:
+        unapply_fixes(host)
 
 
 def uninstall_client(host):
-- 
2.7.4

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

Reply via email to