[Django] #32655: Improve error if a string is passed as an extra_test to DiscoverRunner.build_suite()
Django
DiscoverRunner.build_suite()
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: nobody
Jerdonek |
Type: | Status: new
Cleanup/optimization |
Component: Testing | Version: 4.0
framework | Keywords:
Severity: Normal | iter_test_cases,DiscoverRunner,extra_tests
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32655>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
DiscoverRunner.build_suite()
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
Old description:
New description:
+ # Prevent an unfriendly RecursionError that can happen with
strings.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:1>
Django
DiscoverRunner.build_suite()
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
iter_test_cases,DiscoverRunner,extra_tests| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:2>
Django
DiscoverRunner.build_suite()
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
iter_test_cases,DiscoverRunner,extra_tests| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:3>
Django
DiscoverRunner.build_suite()
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
iter_test_cases,DiscoverRunner,extra_tests|
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
OK, one way or the other, we can do something here. Thanks Chris.
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:4>
Django
DiscoverRunner.build_suite()
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
iter_test_cases,DiscoverRunner,extra_tests|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:5>
Django
DiscoverRunner.build_suite()
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
iter_test_cases,DiscoverRunner,extra_tests|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:6>
Django
DiscoverRunner.build_suite()
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
iter_test_cases,DiscoverRunner,extra_tests|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/14572
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
iter_test_cases,DiscoverRunner,extra_tests|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
iter_test_cases,DiscoverRunner,extra_tests|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"8bca838f4a230b78dd44fc1db3c1b81e444c6099" 8bca838]:
{{{
#!CommitTicketReference repository=""
revision="8bca838f4a230b78dd44fc1db3c1b81e444c6099"
Refs #32655 -- Improved error if iter_test_cases() is passed a string.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
iter_test_cases,DiscoverRunner,extra_tests|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
iter_test_cases,DiscoverRunner,extra_tests|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Jacob Walls
* status: new => assigned
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/14647 Deprecation PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
iter_test_cases,DiscoverRunner,extra_tests|
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
iter_test_cases,DiscoverRunner,extra_tests|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
iter_test_cases,DiscoverRunner,extra_tests| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Jacob
Type: | Walls
Component: Testing framework | Version: 4.0
Keywords: | Triage Stage: Ready for
iter_test_cases,DiscoverRunner,extra_tests| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"56f9579105c324ff15250423bf9f8bdf1634cfb4" 56f9579]:
{{{
#!CommitTicketReference repository=""
revision="56f9579105c324ff15250423bf9f8bdf1634cfb4"
Fixed #32655 -- Deprecated extra_tests argument for
DiscoverRunner.build_suite()/run_tests().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: closed
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
iter_test_cases,DiscoverRunner,extra_tests| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"43b01300b7c7e9d24b549a408010e52a09726060" 43b01300]:
{{{
#!CommitTicketReference repository=""
revision="43b01300b7c7e9d24b549a408010e52a09726060"
Refs #32655 -- Removed extra_tests argument for
DiscoverRunner.build_suite()/run_tests().
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32655#comment:16>