#32655: Improve error if a string is passed as an extra_test to
DiscoverRunner.build_suite()
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
iter_test_cases,DiscoverRunner,extra_tests| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Chris Jerdonek:
Old description:
> 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
> [https://github.com/django/django/blob/23fa29f6a6659e0f600d216de6bcb79e7f6818c9/django/test/utils.py#L238-L249
> 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):
> + 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
> }}}
New description:
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
[https://github.com/django/django/blob/23fa29f6a6659e0f600d216de6bcb79e7f6818c9/django/test/utils.py#L238-L249
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
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.fb7aa9ad01cc93c82ddb00ef8c951fc6%40djangoproject.com.
