[Freeipa-devel] [PATCHES] 0656-0673 Switch the test suite to pytest
Petr Viktorin
pviktori at redhat.com
Mon Nov 24 08:57:01 UTC 2014
On 11/21/2014 10:13 PM, Rob Crittenden wrote:
> Tomas Babej wrote:
>>
>> On 11/20/2014 10:12 AM, Petr Viktorin wrote:
>>> On 11/19/2014 01:11 PM, Tomas Babej wrote:
>>>>
>>>> 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.
>>>>
>>>
>>> Rebased onto master.
>>>
>>
>> I've read through the patches, they seem ok now. Thanks for fixing the
>> issues from the first iterations.
>>
>> ACK.
>>
>> Pushed to master: d42c26c542cfa02c6ba3465ce529ef0cc8f37eda
>>
>
> How do you debug with tests with pytest? ipa-run-tests -s doesn't stop
> on pdb embedded in tree.
>
> rob
Use --pdb instead of -s.
(See --help output.)
--
Petr³
More information about the Freeipa-devel
mailing list