[Freeipa-devel] [PATCHES] 0656-0673 Switch the test suite to pytest

Wed Oct 29 15:52:09 UTC 2014

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 at thinkpad7 ipatests]$ git grep setUpClass
>      [tbabej at thinkpad7 ipatests]$ git grep tearDownClass
>      [tbabej at 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 at 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.

-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0656.3-tests-Use-PEP8-compliant-setup-teardown-method-names.patch
Type: text/x-patch
Size: 20247 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0657.3-tests-Add-configuration-for-pytest.patch
Type: text/x-patch
Size: 781 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0658.3-ipatests.util.ClassChecker-Raise-AttributeError-in-g.patch
Type: text/x-patch
Size: 878 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0659.3-test_automount_plugin-Fix-test-ordering.patch
Type: text/x-patch
Size: 1605 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0660.3-Use-setup_class-teardown_class-in-Declarative-tests.patch
Type: text/x-patch
Size: 4907 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0661.3-dogtag-plugin-Don-t-use-doctest-syntax-for-non-docte.patch
Type: text/x-patch
Size: 2523 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0662.3-test_webui-Don-t-use-__init__-for-test-classes.patch
Type: text/x-patch
Size: 6780 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0663.3-test_ipapython-Use-functions-instead-of-classes-in-t.patch
Type: text/x-patch
Size: 3192 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0664.3-Configure-pytest-to-run-doctests.patch
Type: text/x-patch
Size: 3135 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0665.3-Declarative-tests-Move-cleanup-to-setup_class-teardo.patch
Type: text/x-patch
Size: 2829 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0666.3-Declarative-tests-Switch-to-pytest.patch
Type: text/x-patch
Size: 5130 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0667.3-Integration-tests-Port-the-ordering-plugin-to-pytest.patch
Type: text/x-patch
Size: 12864 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0668.3-Switch-make-test-to-pytest.patch
Type: text/x-patch
Size: 2058 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0669.3-Add-local-pytest-plugin-for-with-xunit-and-logging-l.patch
Type: text/x-patch
Size: 4507 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0670.3-Switch-ipa-run-tests-to-pytest.patch
Type: text/x-patch
Size: 3002 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0671.3-Switch-integration-testing-config-to-a-fixture.patch
Type: text/x-patch
Size: 8108 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0672.3-Integration-tests-Port-the-BeakerLib-plugin-and-log-.patch
Type: text/x-patch
Size: 30089 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0673.3-test_integration-Adjust-tests-for-pytest.patch
Type: text/x-patch
Size: 5933 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141029/87cedfb7/attachment-0017.bin>