Opened 5 years ago
Closed 5 years ago
#32540 closed Cleanup/optimization (fixed)
Only do "top level" detection in DiscoverRunner.build_suite() when needed
| Reported by: | Chris Jerdonek | Owned by: | Chris Jerdonek | 
|---|---|---|---|
| Component: | Testing framework | Version: | dev | 
| Severity: | Normal | Keywords: | DiscoverRunner, build_suite, discovery | 
| 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 )
Currently, DiscoverRunner.build_suite() is a bit confusing to read because it does "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 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.
Change History (7)
comment:1 by , 5 years ago
| Description: | modified (diff) | 
|---|
comment:2 by , 5 years ago
| Triage Stage: | Unreviewed → Accepted | 
|---|
comment:3 by , 5 years ago
| Owner: | changed from to | 
|---|---|
| Status: | new → assigned | 
comment:4 by , 5 years ago
| Has patch: | set | 
|---|
comment:5 by , 5 years ago
| Triage Stage: | Accepted → Ready for checkin | 
|---|
PR: https://github.com/django/django/pull/14120