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