Deprecate DiscoverRunner.build_suite()'s extra_tests argument.

[Chris Jerdonek]

Currently, if you pass a string like 'abc' rather than a TestCase instance as one of the items in the extra_tests argument to DiscoverRunner.build_suite(), you will get a not-too-helpful error that looks something like this:

Traceback (most recent call last):
  File "./runtests.py", line 593, in <module>
    options.timing,
  File "./runtests.py", line 325, in django_tests
    failures = test_runner.run_tests(test_labels or get_installed())
  File ".../django/test/runner.py", line 759, in run_tests
    suite = self.build_suite(test_labels, extra_tests)
  File ".../django/test/runner.py", line 641, in build_suite
    all_tests.extend(iter_test_cases(extra_tests))
  File ".../django/test/utils.py", line 249, in iter_test_cases
    yield from iter_test_cases(test)
  File ".../django/test/utils.py", line 249, in iter_test_cases
    yield from iter_test_cases(test)
  File ".../django/test/utils.py", line 249, in iter_test_cases
    yield from iter_test_cases(test)
  [Previous line repeated 991 more times]
  File ".../django/test/utils.py", line 245, in iter_test_cases
    if isinstance(test, TestCase):
RecursionError: maximum recursion depth exceeded while calling a Python object

This is because ​iter_test_cases() goes into an infinite loop when it gets to iter_test_cases('a') (iterating the first element of 'abc').

Since passing a string is a plausible mistake, I think it would be worth doing something to short-circuit this case and prevent a RecursionError. The following would accomplish this with a pointer to the problem and without adding too much complexity:

diff --git a/django/test/utils.py b/django/test/utils.py
index e977db8a10..ec4dc1837d 100644
--- a/django/test/utils.py
+++ b/django/test/utils.py
@@ -244,6 +244,8 @@ def iter_test_cases(tests):
     for test in tests:
         if isinstance(test, TestCase):
             yield test
+        elif isinstance(test, str):
+            # Prevent an unfriendly RecursionError that can happen with strings.
+            raise TypeError(f'test {test!r} must be a test case or test suite not string')
         else:
             # Otherwise, assume it is a test suite.
             yield from iter_test_cases(test)

It would result in the following:

Traceback (most recent call last):
  File "./runtests.py", line 593, in <module>
    options.timing,
  File "./runtests.py", line 325, in django_tests
    failures = test_runner.run_tests(test_labels or get_installed())
  File ".../django/test/runner.py", line 758, in run_tests
    suite = self.build_suite(test_labels, extra_tests)
  File ".../django/test/runner.py", line 641, in build_suite
    all_tests.extend(iter_test_cases(extra_tests))
  File ".../django/test/utils.py", line 248, in iter_test_cases
    raise TypeError(f'test {test!r} must be a test case or test suite not string')
TypeError: test 'abc' must be a test case or test suite not string

Note that prior to #32489 ("Add a generator function in runner.py to iterate over a test suite's test cases"), the error looked like the following, which was better than what it is now, but not quite as good as what I propose above:

Traceback (most recent call last):
  File "./runtests.py", line 593, in <module>
    options.timing,
  File "./runtests.py", line 325, in django_tests
    failures = test_runner.run_tests(test_labels or get_installed())
  File ".../django/test/runner.py", line 724, in run_tests
    suite = self.build_suite(test_labels, extra_tests)
  File ".../django/test/runner.py", line 615, in build_suite
    suite.addTest(test)
  File ".../unittest/suite.py", line 47, in addTest
    raise TypeError("{} is not callable".format(repr(test)))
TypeError: 'abc' is not callable

[Chris Jerdonek]

Note that this issue was inspired by this error coming up in real-life here: ​https://github.com/django/django/pull/14261#issuecomment-819546454

(The person didn't know why the error was happening.)

[Mariusz Felisiak]

extra_tests is unused since 1e72b1c5c11d1d2dc3ce3660a1eb6b775dcca5a5. Maybe we should deprecate it (and remove in Django 5.0), I don't see much value in keeping this argument.

[Carlton Gibson]

OK, one way or the other, we can do something here. Thanks Chris.

[Chris Jerdonek]

Thanks. A couple thoughts: Before deprecating extra_tests, we might want to think through what the alternative escape hatch is for users that might want to add tests not currently available via discovery. Though iter_test_cases() is an internal function, having the extra check could still help to avoid some puzzling moments.

[Mariusz Felisiak]

Thanks. A couple thoughts: Before deprecating extra_tests, we might want to think through what the alternative escape hatch is for users that might want to add tests not currently available via discovery.

They can always subclass DiscoverRunner. extra_tests is not used internally and it's untested. I don't think we need to provide any alternative. We can always ask for opinions on the DevelopersMailingList.

Though iter_test_cases() is an internal function, having the extra check could still help to avoid some puzzling moments.

Agreed.

[Chris Jerdonek]

PR: ​https://github.com/django/django/pull/14572

[Mariusz Felisiak <felisiak.mariusz@…>]

In 8bca838:

Refs #32655 -- Improved error if iter_test_cases() is passed a string.

[Jacob Walls]

​Deprecation PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 56f9579:

Fixed #32655 -- Deprecated extra_tests argument for DiscoverRunner.build_suite()/run_tests().

[Mariusz Felisiak <felisiak.mariusz@…>]

In 43b01300:

Refs #32655 -- Removed extra_tests argument for DiscoverRunner.build_suite()/run_tests().

Per deprecation timeline.