Hi Oleg!

As we discussed this morning there're still some issues [1][2] but I think the patch set is in reasonably good state and I'm not sure that the issues are bugs in IPA or in tests. This can be investigated and fixed later.


ACK.

Pushed to master:
* bbac233b5ee487ab0e035cf0b861144769a0b738 tests: Fixed method failures during second call for the method * 2f6ffa326adb4d4e9152463ffa733d559f7be2af tests: Added basic constraints extension to the CA certs * 0c635686ddc6dbba54285a9c7844e12342083b9b tests: Added generation of missing certs * 38ad864342bb3fcbf65397b763c240f034f3e2c7 tests: Updated ipa server installation stdin text * c0e16aa3b9c380fb7936dc18c4bfc04e7f8327b5 tests: Create a method that cleans all ipa certs * 48ca465a12a91977eb57bb791cf0b098e5e5a4b3 tests: Added teardown methods for server and replica installation * 725d8d0cac16f6a41ad54ae319e3822d44047031 tests: Removed call for install method from parent class * fad6ec8256a97fbf06b3e4509d93af1d159e6b81 tests: Adapted installation methods to utilize methods from tasks * 84db13f676771746f9ab7769073837a29f6de464 tests: Fixed incorrect assert in verify_installation * a81d8472042f4909020c073ecb00a37d8e05ec33 tests: Applied correct teardown methods * 759bbcdfcbeade91c77b201c439c939d6477cd08 tests: Removed outdated command options test * d17d13d77a7c09cbc99c8bb0a3f7af3b72da8aca tests: Added necessary getkeytabs calls to fixtures
* 24f218f4ebe203213eebede59ff79b89c657ee76 tests: Added necessary xfails
* e0b67dfa7e957cc134043b265b87ed71bb09a7d3 tests: Updated master and replica installation methods to enable negative testing * 9217bcc871468615110c85b1131b62735f9e5092 tests: Made unapply_fixes call optional at master uninstallation * bb4205b582038669888544786a5611b18e52bf42 tests: Enabled negative testing for cleaning replication agreements * dbf0d141c5a4d1ccd1b681ac49cb57e47234aaa4 tests: Replaced hardcoded certutil with imported from paths * b8cf212e8bbe301f6d44551ecac063d12a042520 tests: Replaced unused setUp method with install * 804aae81966f23e307ec86364489f808fd0c3357 tests: fixed expects of incorrect error messages * 43994e669743bb8f54e32b52f2410ebde3660f04 tests: Fixed Usage of improper certs in ca-less tests * b8968d923cedbbeb931c3ed33b81b299a55baf4a tests: Implemented check for domainlevel before installation verification * 106f37c26f64492f771214409d229feb5d70a113 tests: Standardized replica_preparation in test_no_certs * 8be0906b04bbf995af6326b560151d15901b544e tests: added verbose assert to test_service_disable_doesnt_revoke * f1f94a7b9fe354d93f31ce8cd606d985dd44703b tests: fixed super method invocation
* 7412f0cb20801e1608f8cf388210e57ef7d27497 tests: fixed certinstall method
* 9870c5804a65ae320ebbcb313f8facb21963f710 tests: Reverted erroneous asserts in 4 tests * 47c808afa35f0708ca00ac8e98851c9f8d75badc tests: Fixed code styling in caless tests to make pep8 happy

[1] https://fedorahosted.org/freeipa/ticket/6346
[2] https://fedorahosted.org/freeipa/ticket/6348

On 22/09/16 12:55, Oleg Fayans wrote:
Fixed patch N 41

On 09/21/2016 04:21 PM, Oleg Fayans wrote:
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
























--
David Kupka

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