Opened 5 years ago

Closed 5 years ago

#29658 closed Cleanup/optimization (fixed)

Use a context manager to unregister model field lookups in tests

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 (9)

comment:1 Changed 5 years ago by Claude Paroz

Triage Stage: Unreviewed → Accepted

comment:2 Changed 5 years ago by Dominic White

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

comment:3 Changed 5 years ago by Srinivas Reddy Thatiparthy

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

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

comment:4 Changed 5 years ago by Srinivas Reddy Thatiparthy

Owner: changed from Dominic White to Srinivas Reddy Thatiparthy

comment:5 Changed 5 years ago by Claude Paroz

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 Changed 5 years ago by Tim Graham

Triage Stage: Accepted → Ready for checkin

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

comment:7 Changed 5 years ago by Simon Charette

Using context managers should be fine as well.

comment:8 Changed 5 years ago by Tim Graham

Description: modified (diff)
Summary: Use unittest.addCleanup to unregister lookups. → Use a context manager to unregister model field lookups in tests

comment:9 Changed 5 years ago by Tim Graham <timograham@…>

Resolution: → fixed
Status: assigned → closed

In 233c70f0:

Fixed #29658 -- Registered model lookups in tests with a context manager.

Note: See TracTickets for help on using tickets.
Back to Top