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

How do you debug with tests with pytest? ipa-run-tests -s doesn't stop
on pdb embedded in tree.

rob

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to