Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#32489 closed Cleanup/optimization (fixed)

Add a generator function in runner.py to iterate over a test suite's test cases

Reported by: Chris Jerdonek Owned by: Chris Jerdonek
Component: Testing framework Version: dev
Severity: Normal Keywords: test suite, iterator,
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Chris Jerdonek)

There are four functions or methods in test/runner.py that need to iterate over a test suite's test cases:

Each of these functions is a little harder to understand than it needs to be because each needs to contain the logic of how to iterate over a test suite, which isn't obvious. In particular, they're all implemented as recursive functions, since test suites can contain test suites.

If runner.py contained a single helper function that accepts a test suite instance and returns an iterator of the test suite's test cases, then each of those four functions could be simplified. They could contain a simple for loop over the iterator's return value and not have to be recursive.

The helper function would be pretty simple and could itself be recursive (but it wouldn't need to be). It could be called something like iter_test_cases(suite).

Change History (14)

comment:1 by Chris Jerdonek, 4 years ago

Description: modified (diff)

comment:2 by Chris Jerdonek, 4 years ago

If the function were implemented recursively, I think it could be something like this (untested):

def iter_test_cases(suite):
    """Return an iterator over a test suite's unittest.TestCase objects."""
    # Django permits custom TestSuite classes, so use the caller's class.
    suite_class = type(suite)
    for test in suite:
        if isinstance(test, suite_class):
            yield from iter_test_cases(test)
        else:
            yield test
Last edited 4 years ago by Chris Jerdonek (previous) (diff)

comment:3 by Nick Pope, 4 years ago

Triage Stage: UnreviewedAccepted

Sounds sensible to me. As you say, these are all just slightly different enough to be hard to understand.

You proposed implementation seems fine. We might just need to be careful with the groupby in partition_suite_by_case().

comment:4 by Chris Jerdonek, 4 years ago

Thanks. To lower the bar for PR's, I would suggest that the first PR could start by adding the function and changing just one of the existing functions to use it (e.g. partition_suite_by_type()). Subsequent PR's could change one or more of the other functions to use it.

comment:5 by Chris Jerdonek, 4 years ago

Also, I just noticed that the proposed iter_test_cases() should probably also sport a reverse keyword argument, to support e.g. partition_suite_by_type(). That way partition_suite_by_type() won't need to construct the full list before reversing it.

The new proposed implementation of iter_test_cases() would be (following the pattern of partition_suite_by_type()):

def iter_test_cases(suite, reverse=False):
    """Return an iterator over a test suite's unittest.TestCase objects."""
    # Django permits custom TestSuite classes, so use the caller's class.
    suite_class = type(suite)
    if reverse:
        suite = reversed(tuple(suite))
    for test in suite:
        if isinstance(test, suite_class):
            yield from iter_test_cases(test, reverse=reverse)
        else:
            yield test
Last edited 4 years ago by Chris Jerdonek (previous) (diff)

comment:6 by Chris Jerdonek, 4 years ago

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:7 by Chris Jerdonek, 4 years ago

Has patch: set

comment:8 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 22c9af0e:

Fixed #32489 -- Added iter_test_cases() to iterate over a TestSuite.

This also makes partition_suite_by_type(), partition_suite_by_case(),
filter_tests_by_tags(), and DiscoverRunner._get_databases() to use
iter_test_cases().

comment:10 by Chris Jerdonek, 4 years ago

I posted a follow-up PR with some additional simplifications made possible by the change in the first PR:
https://github.com/django/django/pull/14085

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In cc12894:

Refs #32489 -- Removed unneeded partition_suite_by_type().

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 8ff75a3:

Refs #32489 -- Simplified partition_suite_by_case().

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 465fdffd:

Refs #32489 -- Simplified filter_tests_by_tags().

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In d8a4bcff:

Refs #32489 -- Doc'd and tested iter_test_cases() support for an iterable of tests.

Note: See TracTickets for help on using tickets.
Back to Top