[Freeipa-devel] [PATCHES] 0656-0673 Switch the test suite to pytest
Tomas Babej
tbabej at redhat.com
Wed Nov 19 12:11:03 UTC 2014
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 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.
>
> 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
More information about the Freeipa-devel
mailing list