Opened 6 years ago

Last modified 6 years ago

#29658 closed Cleanup/optimization

Use a context manager to unregister model field lookups in tests — at Version 8

Reported by: Mads Jensen Owned by: Srinivas Reddy Thatiparthy
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Tim Graham)

There are some places in the test suite that follow this pattern:

try:
   CharField.register_lookup(Length)
   # some test code
finally:
   CharField._unregister_lookup(Length)

It'd be better to use a context manager (addCleanup() was originally proposed) to handle this stuff.

Change History (8)

comment:1 by Claude Paroz, 6 years ago

Triage Stage: Unreviewed → Accepted

comment:2 by Dominic White, 6 years ago

Owner: changed from nobody to Dominic White
Status: new → assigned

comment:3 by Srinivas Reddy Thatiparthy, 6 years ago

​https://github.com/django/django/pull/10312

Last edited 6 years ago by Srinivas Reddy Thatiparthy (previous) (diff)

comment:4 by Srinivas Reddy Thatiparthy, 6 years ago

Owner: changed from Dominic White to Srinivas Reddy Thatiparthy

comment:5 by Claude Paroz, 6 years ago

Has patch: set

The current patch is using a context manager instead of the try/finally method.
Mads, is that something you are comfortable with, or do you still see some benefit with addCleanup?

comment:6 by Tim Graham, 6 years ago

Triage Stage: Accepted → Ready for checkin

Patch looks good to me, unless there's some reason to prefer addCleanup().

comment:7 by Simon Charette, 6 years ago

Using context managers should be fine as well.

comment:8 by Tim Graham, 6 years ago

Description: modified (diff)
Summary: Use unittest.addCleanup to unregister lookups. → Use a context manager to unregister model field lookups in tests
Note: See TracTickets for help on using tickets.
Back to Top