On 11/14/2014 09:55 AM, Petr Viktorin wrote: > On 10/29/2014 04:52 PM, Petr Viktorin wrote: >> On 10/29/2014 01:22 PM, Tomas Babej wrote: >>> >>> On 10/27/2014 04:38 PM, Petr Viktorin wrote: >>>> On 10/15/2014 02:58 PM, Petr Viktorin wrote: >>>>> This almost completes the switch to pytest. There are two missing >>>>> things: >>>>> - the details of test results (--with-xunit) are not read >>>>> correctly by >>>>> Jenkins. I have a theory I'm investigating here. >>>>> - the beakerlib integration is still not ready >>>>> >>>>> >>>>> I'll not be available for the rest of the week so I'm sending this >>>>> early, in case someone wants to take a look. >>>> >>>> I've updated (and rearranged) the patches after some more testing. >>>> Both points above are fixed. Individual plugins are broken out; some >>>> would be nice to even release independently of IPA. (There is some >>>> demand for the BeakerLib plugin; for that I'd only need to break the >>>> dependency on ipa_log_manager.) >>>> >>>> >>>> These depend on my patches 0656-0660. >>>> >> >>> Thanks for this effort! >>> >>> #### Patch 0656: tests: Use PEP8-compliant setup/teardown method names >>> >>> There are some references to the old names in the test_ipapython and >>> test_install: >>> >>> [tbabej@thinkpad7 ipatests]$ git grep setUpClass >>> [tbabej@thinkpad7 ipatests]$ git grep tearDownClass >>> [tbabej@thinkpad7 ipatests]$ git grep setUp >>> test_install/test_updates.py: def setUp(self): >>> test_ipapython/test_cookie.py: def setUp(self): >>> test_ipapython/test_cookie.py: def setUp(self): >>> test_ipapython/test_cookie.py: def setUp(self): >>> test_ipapython/test_dn.py: def setUp(self): >>> test_ipapython/test_dn.py: def setUp(self): >>> test_ipapython/test_dn.py: def setUp(self): >>> test_ipapython/test_dn.py: def setUp(self): >>> test_ipapython/test_dn.py: def setUp(self): >>> [tbabej@thinkpad7 ipatests]$ git grep tearDown >>> test_install/test_updates.py: def tearDown(self): >> >> These are in unittest.testCase. It would be nice to convert those as >> well, but that's for a larger cleanup. >> >>> #### Patch 0657: tests: Add configuration for pytest >>> >>> Shouldn't we rather change the class names? >> >> Ideally yes, but I don't think renaming most of our test classes would >> be worth the benefit. >> >>> #### Patch 0658: ipatests.util.ClassChecker: Raise AttributeError in >>> get_subcls >>> >>> ACK. >>> >>> #### Patch 0659: test_automount_plugin: Fix test ordering >>> >>> ACK. >>> >>> #### PATCH 0660: Use setup_class/teardown_class in Declarative tests >>> >>> --- a/ipatests/test_ipaserver/test_changepw.py >>> +++ b/ipatests/test_ipaserver/test_changepw.py >>> @@ -33,8 +33,7 @@ >>> class test_changepw(XMLRPC_test, Unauthorized_HTTP_test): >>> app_uri = '/ipa/session/change_password' >>> >>> - def setup(self): >>> - super(test_changepw, self).setup() >>> + def setup(cls): >>> try: >>> api.Command['user_add'](uid=testuser, >>> givenname=u'Test', sn=u'User') >>> api.Command['passwd'](testuser, >>> password=u'old_password') >>> @@ -43,12 +42,11 @@ def setup(self): >>> 'Cannot set up test user: %s' % e >>> ) >>> >>> - def teardown(self): >>> + def teardown(cls): >>> try: >>> api.Command['user_del']([testuser]) >>> except errors.NotFound: >>> pass >>> - super(test_changepw, self).teardown() >>> >>> The setup/teardown methods are not classmethods, so the name of the >>> first argument should remain "self". >> >> Oops, thanks for the catch! >> >>> #### PATCH 0661: dogtag plugin: Don't use doctest syntax for >>> non-doctest >>> examples >>> >>> ACK. >>> >>> #### PATCH 0662: test_webui: Don't use __init__ for test classes >>> >>> I don't think the following change will work: >>> >>> - def __init__(self, driver=None, config=None): >>> + def setup(self, driver=None, config=None): >>> self.request_timeout = 30 >>> self.driver = driver >>> self.config = config >>> if not config: >>> self.load_config() >>> + self.get_driver().maximize_window() >>> + >>> + def teardown(self): >>> + self.driver.quit() >>> >>> def load_config(self): >>> """ >>> @@ -161,20 +165,6 @@ def load_config(self): >>> if 'type' not in c: >>> c['type'] = DEFAULT_TYPE >>> >>> - def setup(self): >>> - """ >>> - Test setup >>> - """ >>> - if not self.driver: >>> - self.driver = self.get_driver() >>> - self.driver.maximize_window() >>> - >>> - def teardown(self): >>> - """ >>> - Test clean up >>> - """ >>> - self.driver.quit() >>> >>> >>> The setup(self) method used to set the self.driver attribute on the >>> class instance, now nothing sets it. The setup method should do: >>> >>> def setup(self, driver=None, config=None): >>> self.request_timeout = 30 >>> self.driver = driver >>> self.config = config >>> if not config: >>> self.load_config() >>> if not self.driver: >>> self.driver = self.get_driver() >>> self.driver.maximize_window() >>> >>> However, I would prefer having self.driver as a cached property. >> >> Well, ideally these would be fixtures. >> >> Thanks for the catch. >> >>> #### PATCH 0663: test_ipapython: Use functions instead of classes in >>> test generators >>> >>> ACK. >>> >>> #### PATCH 0664: Configure pytest to run doctests >>> >>> ACK. >>> >>> #### PATCH 0665: Declarative tests: Move cleanup to >>> setup_class/teardown_class >>> >>> You should also remove the now redundant cleanup_generate method. >> >> Right, removed. >> >>> Also: Do we really want to print out error.NotFound or >>> errors.EmptyModList exceptions produced by cleanup commands? >> >> It's just one line, might help debugging. It should only show up on >> failures. >> >>> #### PATCH 0666: Declarative tests: Switch to pytest >>> >>> Nitpitck: declarative.py has wrong year in the copyright header. >>> Otherwise ACK. >> >> Fixed. >> >>> #### PATCH 0667: Integration tests: Port the ordering plugin to pytest >>> >>> Specfile change belongs to the previous patch, but I doubt these would >>> be separated, so not worth spending the time IMHO. >> >> Also, the previous patch doesn't actually import pytest. But a lot of >> these patches break tests by themselves, so they shouldn't be separated. >> >>> Why does TestIPACommands have ordered decorator applied? It is applied >>> on IntegrationTest directly, from which CALessBase inherits from. I >>> don't think it's necessary (unless I'm missing something), so we should >>> rather remove the occurence of the decorator in the test_caless.py than >>> fixing the import. >> >> Actually, I think only only the classes that actually need it should >> have the decorator. >> Again, cleanup for a future time. >> >>> #### PATCH 0668: Switch make-test to pytest >>> >>> Nice simplification. However, make-test does not work for me: >>> >>> I'm getting tons of errors (during the collection) along the lines of: >>> >>> ________________ ERROR collecting ipatests/test_xmlrpc/testcert.py >>> _________________ >>> /usr/lib/python2.7/site-packages/py/_path/local.py:661: in >>> pyimport >>> raise self.ImportMismatchError(modname, modfile, self) >>> E ImportMismatchError: ('ipatests.test_xmlrpc.testcert', >>> '/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/testcert.py', >>> local('/ipadev/freeipa/ipatests/test_xmlrpc/testcert.py')) >>> _______________ ERROR collecting >>> ipatests/test_xmlrpc/xmlrpc_test.py _______________ >>> /usr/lib/python2.7/site-packages/py/_path/local.py:661: in >>> pyimport >>> raise self.ImportMismatchError(modname, modfile, self) >>> E ImportMismatchError: ('ipatests.test_xmlrpc.xmlrpc_test', >>> '/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py', >>> local('/ipadev/freeipa/ipatests/test_xmlrpc/xmlrpc_test.py')) >>> _______________ ERROR collecting >>> ipatests/test_xmlrpc/xmlrpc_test.py _______________ >>> import file mismatch: >>> imported module 'ipatests.test_xmlrpc.xmlrpc_test' has this >>> __file__ attribute: >>> /usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py >>> which is not the same as the test file we want to collect: >>> /ipadev/freeipa/ipatests/test_xmlrpc/xmlrpc_test.py >>> HINT: remove __pycache__ / .pyc files and/or use a unique basename >>> for your test file modules >>> ================ 163 passed, 7 skipped, 679 error in 48.14 seconds >>> ============== >>> >>> >>> I am running this as ./make-test from the directory containing the >>> freeipa sources. The ipa-run-tests command seems to work fine though. >> >> Hm. Doesn't happen for me. Did you follow the hint (git clean -fxd)? >> I've added . to PYTHONPATH, that may help (and it should also help with >> plugin loading in some cases). >> >>> #### PATCH 0669: Add local pytest plugin for --with-xunit and >>> --logging-level >>> >>> ACK. >>> >>> #### PATCH 0670: Switch ipa-run-tests to pytest >>> >>> ACK. >>> >>> #### PATCH 0671: Switch integration testing config to a fixture >>> >>> ACK. >>> >>> #### PATCH 0672: Integration tests: Port the BeakerLib plugin and log >>> collection to pytest >>> >>> I didn't see any glitches, but I haven't tested this one out yet >>> functionally. >>> >>> ### General notes >>> >>> I did two IPA master installs today, one with your patches applied. >>> There were significant differences in the number of successfully >>> passing >>> tests: >>> >>> nose: FAILED (SKIP=60, errors=46, failures=14) >>> pytest: ========= 42 failed, 1830 passed, 422 skipped, 340 error in >>> 860.47 seconds ========= >>> >>> Did you not encounter this in your testing? I will look more deeply >>> into >>> the failures themselves. >> >> I did, but it was always some stupid mistake :) >> Do you have ~/.ipa set up, and $MASTER undefined? >> >> The integration test did fail for me now when they were not configured. >> I added an additional patch to fix that. > > Ping, could you look at the updated patches? > >
Sure, but the patchset requires a rebase. -- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel