[Django] #32540: Only do "top level" detection in DiscoverRunner.build

Django

unread,

Mar 12, 2021, 2:23:38 AM3/12/21

to django-...@googlegroups.com

#32540: Only do "top level" detection in DiscoverRunner.build_suite() when needed
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: nobody
Jerdonek |
Type: | Status: new
Cleanup/optimization |
Component: Testing | Version: dev
framework | Keywords:
Severity: Normal | DiscoverRunner,build_suite,discovery
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Currently, `DiscoverRunner.build_suite()` is a bit confusing to read
because it does
[https://github.com/django/django/blob/551b0f94bf62c81d9ff9d5f7e261bcd2a594a4d1/django/test/runner.py#L577-L599
"top level" detection] for test discovery even when discovery won't be
taking place.

My suggestion is to move the top-level detection logic (and large code
comment) into a `find_top_level(top_level)` function that is placed
immediately before where `is_discoverable()` is defined. Then, only call
`find_top_level()` when needed, namely right before where
[https://github.com/django/django/blob/551b0f94bf62c81d9ff9d5f7e261bcd2a594a4d1/django/test/runner.py#L603
self.test_loader.discover()] is called.

Similarly, the `kwargs = discover_kwargs.copy()` line can also go into
that if block right before that line, because that's the only code path
where `kwargs` is actually used. Without this change, it's hard for one to
notice that the `kwargs` are in fact only used for that one line.

Together, this will cut down on the size of `build_suite()` quite a bit
and make it easier to understand.

--
Ticket URL: <https://code.djangoproject.com/ticket/32540>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Mar 12, 2021, 2:24:45 AM3/12/21

to django-...@googlegroups.com

#32540: Only do "top level" detection in DiscoverRunner.build_suite() when needed
-------------------------------------+-------------------------------------

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Chris Jerdonek:

Old description:

> Currently, `DiscoverRunner.build_suite()` is a bit confusing to read
> because it does
> [https://github.com/django/django/blob/551b0f94bf62c81d9ff9d5f7e261bcd2a594a4d1/django/test/runner.py#L577-L599
> "top level" detection] for test discovery even when discovery won't be
> taking place.
>
> My suggestion is to move the top-level detection logic (and large code
> comment) into a `find_top_level(top_level)` function that is placed
> immediately before where `is_discoverable()` is defined. Then, only call
> `find_top_level()` when needed, namely right before where
> [https://github.com/django/django/blob/551b0f94bf62c81d9ff9d5f7e261bcd2a594a4d1/django/test/runner.py#L603
> self.test_loader.discover()] is called.
>
> Similarly, the `kwargs = discover_kwargs.copy()` line can also go into
> that if block right before that line, because that's the only code path
> where `kwargs` is actually used. Without this change, it's hard for one
> to notice that the `kwargs` are in fact only used for that one line.
>
> Together, this will cut down on the size of `build_suite()` quite a bit
> and make it easier to understand.

New description:

Currently, `DiscoverRunner.build_suite()` is a bit confusing to read
because it does

[https://github.com/django/django/blob/551b0f94bf62c81d9ff9d5f7e261bcd2a594a4d1/django/test/runner.py#L576-L599

"top level" detection] for test discovery even when discovery won't be
taking place.

My suggestion is to move the top-level detection logic (and large code
comment) into a `find_top_level(top_level)` function that is placed
immediately before where `is_discoverable()` is defined. Then, only call
`find_top_level()` when needed, namely right before where
[https://github.com/django/django/blob/551b0f94bf62c81d9ff9d5f7e261bcd2a594a4d1/django/test/runner.py#L603
self.test_loader.discover()] is called.

Similarly, the `kwargs = discover_kwargs.copy()` line can also go into
that if block right before that line, because that's the only code path
where `kwargs` is actually used. Without this change, it's hard for one to
notice that the `kwargs` are in fact only used for that one line.

Together, this will cut down on the size of `build_suite()` quite a bit
and make it easier to understand.

--

--
Ticket URL: <https://code.djangoproject.com/ticket/32540#comment:1>

Django

unread,

Mar 12, 2021, 3:33:41 AM3/12/21

to django-...@googlegroups.com

#32540: Only do "top level" detection in DiscoverRunner.build_suite() when needed
-------------------------------------+-------------------------------------

Reporter: Chris Jerdonek | Owner: nobody
Type: | Status: new
Cleanup/optimization |

Component: Testing framework | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted
DiscoverRunner,build_suite,discovery|
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/32540#comment:2>

Django

unread,

Mar 12, 2021, 4:36:29 AM3/12/21

to django-...@googlegroups.com

#32540: Only do "top level" detection in DiscoverRunner.build_suite() when needed
-------------------------------------+-------------------------------------

Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned

Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
DiscoverRunner,build_suite,discovery|

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Chris Jerdonek):

* owner: nobody => Chris Jerdonek
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/32540#comment:3>

Django

unread,

Mar 13, 2021, 3:00:03 AM3/13/21

to django-...@googlegroups.com

#32540: Only do "top level" detection in DiscoverRunner.build_suite() when needed
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Chris Jerdonek):

* has_patch: 0 => 1

Comment:

PR: https://github.com/django/django/pull/14120

--
Ticket URL: <https://code.djangoproject.com/ticket/32540#comment:4>

Django

unread,

Mar 15, 2021, 5:17:41 AM3/15/21

to django-...@googlegroups.com

#32540: Only do "top level" detection in DiscoverRunner.build_suite() when needed
-------------------------------------+-------------------------------------

Keywords: | Triage Stage: Ready for
DiscoverRunner,build_suite,discovery| checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/32540#comment:5>

Django

unread,

Mar 15, 2021, 6:43:02 AM3/15/21

to django-...@googlegroups.com

#32540: Only do "top level" detection in DiscoverRunner.build_suite() when needed
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"d5a214c7c4575773e0f1b8b99f6018bcb7adb8cd" d5a214c7]:
{{{
#!CommitTicketReference repository=""
revision="d5a214c7c4575773e0f1b8b99f6018bcb7adb8cd"
Refs #32540 -- Added django.test.runner.find_top_level().
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/32540#comment:6>

Django

unread,

Mar 15, 2021, 6:43:02 AM3/15/21

to django-...@googlegroups.com

#32540: Only do "top level" detection in DiscoverRunner.build_suite() when needed
-------------------------------------+-------------------------------------

Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek

Cleanup/optimization | Status: closed

Component: Testing framework | Version: dev

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
DiscoverRunner,build_suite,discovery| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"7bdd09d016f418719f2d0297f58bd81c5349101d" 7bdd09d]:
{{{
#!CommitTicketReference repository=""
revision="7bdd09d016f418719f2d0297f58bd81c5349101d"
Fixed #32540 -- Optimized DiscoverRunner.build_suite() by calling
find_top_level() only if is_discoverable() is true.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/32540#comment:7>

Reply all

Reply to author

Forward

[Django] #32540: Only do "top level" detection in DiscoverRunner.build_suite() when needed

Django

Django

Django

Django

Django

Django

Django

Django