python / cpython

The Python programming language
https://www.python.org
Other
63.11k stars 30.22k forks source link

Run Tkinter tests with wantobjects=False #65784

Open serhiy-storchaka opened 10 years ago

serhiy-storchaka commented 10 years ago
BPO 21585
Nosy @terryjreedy, @zware, @serhiy-storchaka
Dependencies
  • bpo-22236: Do not use _default_root in Tkinter tests
  • Files
  • test_wantobj.patch
  • test_ttk_guionly_wantobj.py
  • wantobj_test.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = 'https://github.com/serhiy-storchaka' closed_at = None created_at = labels = ['tests', 'type-feature', 'expert-tkinter'] title = 'Run Tkinter tests with wantobjects=False' updated_at = user = 'https://github.com/serhiy-storchaka' ``` bugs.python.org fields: ```python activity = actor = 'zach.ware' assignee = 'serhiy.storchaka' closed = False closed_date = None closer = None components = ['Tests', 'Tkinter'] creation = creator = 'serhiy.storchaka' dependencies = ['22236'] files = ['35394', '35395', '36291'] hgrepos = [] issue_num = 21585 keywords = ['patch', 'needs review'] message_count = 11.0 messages = ['219178', '219322', '219323', '224001', '224962', '225478', '225617', '225635', '225642', '225746', '225759'] nosy_count = 6.0 nosy_names = ['terry.reedy', 'gpolo', 'jesstess', 'zach.ware', 'serhiy.storchaka', 'Lita.Cho'] pr_nums = [] priority = 'normal' resolution = None stage = 'needs patch' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue21585' versions = ['Python 3.5'] ```

    serhiy-storchaka commented 10 years ago

    The wantobjects attribute of the tkinter module is True by default, and all Tkinter tests run with wantobjects=True. We need to test Tkinter with wantobjects=False too. All Tkinter tests should be ran twice, in both mode.

    3c449b37-58db-4fad-a8ce-48269629ff90 commented 10 years ago

    So I don't know what the best way to do this, but I changed the widget_tests.py in order to set tkinter.wantobjects = 0 if a 'wantobjects' flag was passed through test.support.use_resources.

    Then I added a new test called test_ttk_guionly_wantobj, where it turns on wantobjects. If we create a new module, then we start off with a clean tkinter object.

    Before I go this route for all the tests, I wanted to make sure if this was the correct way to go about it.

    3c449b37-58db-4fad-a8ce-48269629ff90 commented 10 years ago

    Patches lived in my Linux machine. I've attached my patch. I will add my module next.

    serhiy-storchaka commented 10 years ago

    Thanks Lita, but I think it would be better if all Tkinter tests will automatically run in both wantobjects=0 and wantobjects=1 modes.

    As for your patch, ttk.Button() creates default root with current wantobjects value, this default root can be unintentionally used in tests with other wantobjects value. We should ensure that default root is not used in tests.

    3c449b37-58db-4fad-a8ce-48269629ff90 commented 10 years ago

    Hi Serhiy,

    This patch was made while I was learning tkinter. I figured out how to run the tests twice while changing wantobjects variable without creating new tests. Fortunately, all the tests seem to fast when wantobjects is 0 or 1.

    The only annoying thing is that it doesn't merge the count of the amount of tests ran within that module, since I am calling support.run_unittest twice. Hopefully, that's okay. Otherwise, I need to figure out how to call the tests twice individually within the generator.

    terryjreedy commented 10 years ago

    My impression is that we are trying to move away from using test_main, or is using support.run_unittest considered a sufficient change from the old explicit suite method?

    Anyway, here is an alternate approach, adding two lines to wrap all test classes, that tags the test classes in the code and hence in error messages.

    import unittest
    for i in range(2): exec('''
    class T{0}(unittest.TestCase):
        def test_2(self):
            self.assertTrue({0} == 1)
    '''.format(i))
    unittest.main()
    >>>
    F.

    ====================================================================== FAIL: test_2 (main.T0) ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "<string>", line 4, in test_2
    AssertionError: False is not true

    Ran 2 tests in 0.016s

    3c449b37-58db-4fad-a8ce-48269629ff90 commented 10 years ago

    Hi Terry,

    I had no idea we were moving away from using test_main.

    So instead, of using support.run_unittest, we should import all the unittest from tkinter/test/ and wrap everything with that exec method, setting wantobjects=1 and again with wantobjects=0?

    Also, do you have an example of a unit test that doesn't use test_main? All the unit tests I've seen (tkinter, smtplib, nntplib) all use test_main.

    Lita

    terryjreedy commented 10 years ago

    The current test/test_*.py example file in the docs intentionally does not contain test_main. https://docs.python.org/3/library/test.html#writing-unit-tests-for-the-test-package The next section explaining that test.regrtest runs unittest.TestLoader.loadTestsFromModule if test_main does not exist. I believed support.run_unittest.

    I never used test_main in test_idle.py, Grep test/test_*.py for 'test_main' or its absence to count others. Or search all issues for 'unittest discovery' for conversions.  For instance: python/cpython#62458 made this change to the five test_codecmaps*.py files.
    http://bugs.python.org/file30644/test_codecmaps_discovery.diff
    -def test_main():
    -    support.run_unittest(__name__)
    -
     if __name__ == "__main__":
    -    test_main()
    +    support.use_resources = ['urlfetch']
    +    unittest.main()

    This all said, running all tests in a file with two different values of an attribute of the tested module is an exceptional case. Zach, what do you think?

    zware commented 10 years ago

    I've been thinking on this for a while, and haven't come up with a solution that I like. I haven't had a chance to look at bpo-22236 yet, but I suspect it will help immensely.

    Also, I have a work-in-progress patch for rearranging the tkinter tests into a test.test_tkinter subpackage. At this point, I'm not sure if that will make this issue easier or harder to resolve, but it does take care of not using test_main for the tkinter tests. I'm hoping to have some time to get that patch into a submittable state tomorrow, then have a good look at how it, bpo-22236, and this issue interact. Anyone who wants a look at where I'm going with the test_tkinter subpackage before I get it submitted, it's at http://hg.python.org/sandbox/zware/log/mv_pkg_tests/ (see the changes by doing "hg diff --git -r default -r mv_pkg_tests Lib/test/test_tkinter"; there are changes to other test packages in that branch as well that are missed by specifying Lib/test/test_tkinter).

    serhiy-storchaka commented 10 years ago

    Here is my plan. First, we should resolve bpo-22236. Otherwise some tests will be run with default root created with old "wantobjects" value. Then we should introduce new test resource name as in Lilo's first patch (but "wantobjects" is not correct name, "testallwantobjectsvalues" is more correct but is too long). Then we should add the load_tests() function in every test file which will generate new test classes for every wantobjects value (0, 1 and 2 if bpo-22214 will be applied) when this resource is enabled.

    Any propositions about new resource name?

    As for rearranging the tkinter tests, lets discuss this in separate issue. It looks orthogonal to this issue.

    zware commented 10 years ago

    I agree that bpo-22236 should be resolved first. As for resource guarding the 'wantobjects = 0' tests, do we really need to? It takes a bit under 20 seconds to run all of the tkinter tests on this machine, and I don't think 40 seconds is an unduly long runtime for such a large set of tests, so I would vote for just loading both sets of tests every time. Or if others feel the 'wantobjects' test should be guarded, I think we could just reuse the 'cpu' resource (since it's really just time that we're guarding).

    And the rearrangement will of course be a separate issue; I only mentioned it here because that patch takes care of moving away from test_main and support.run_unittest, and may have an impact on how this issue can be resolved.