Fixed #36369: Added tests for FORWARD_PROPERTIES and REVERSE_PROPERTIES. #19484
Conversation
There was a problem hiding this comment.
Thanks for moving this forward. Regarding the test strategy:
- the reporter mentioned being able to remove almost everything from
FORWARD_PROPERTIESand see no test failures. I notice that's still true. - I left a note on the ticket that I suspected
db_returning_fieldswas missing fromFORWARD_PROPERTIES, and I was hoping a test would help clarify. Were you able to look into that?
I think it would be better to spend less effort testing the mechanics of the caching and more effort testing that the correct caches are expired when the interface to clear them (I think it's apps.clear_cache()) is called.
Do you have some ideas about how we might do that? I don't suppose we want to just maintain another list of FORWARD_PROPERTIES and REVERSE_PROPERTIES in the tests, as that's just as error-prone as what we have now.
Maybe you could iterate the values of Options.__dict__ that are instances of cached_property and assert that each cache is expired on apps.clear_cache(). Ideally, a test like that would reveal the failure I get when I replace fields with db_returning_fields in the test currently part of this PR , below -- but without having to know that in advance!
(Not suggesting that the test be updated to this, see above comment)
diff --git a/tests/model_options/test_meta_caching.py b/tests/model_options/test_meta_caching.py
index adec911256..8fdb76febd 100644
--- a/tests/model_options/test_meta_caching.py
+++ b/tests/model_options/test_meta_caching.py
@@ -135,22 +135,27 @@ class ForwardPropertiesCachingTests(SimpleTestCase):
initial_field = models.CharField(max_length=100)
# Access fields to cache them
- _ = DynamicModel._meta.fields
- self.assertIn("fields", DynamicModel._meta.__dict__)
+ _ = DynamicModel._meta.db_returning_fields
+ self.assertIn("db_returning_fields", DynamicModel._meta.__dict__)
# Add a new field
- new_field = models.IntegerField(name="dynamic_field")
+ new_field = models.GeneratedField(
+ expression=models.functions.Ord(models.F("initial_field")),
+ output_field=models.IntegerField(),
+ db_persist=False,
+ )
+ new_field.name = "new_field"
DynamicModel._meta.add_field(new_field)
8000
# Verify the cache was expired
- self.assertNotIn("fields", DynamicModel._meta.__dict__)
+ self.assertNotIn("db_returning_fields", DynamicModel._meta.__dict__)
# Access fields again
- updated_fields = DynamicModel._meta.fields
+ updated_fields = DynamicModel._meta.db_returning_fields
# Verify the new field is included
- field_names = [f.name for f in updated_fields]
- self.assertIn("dynamic_field", field_names)
+ returning_fields = [f.name for f in updated_fields if f.db_returning]
+ self.assertIn("new_field", returning_fields)
@isolate_apps("model_options")Let me know if that makes sense.
| self.assertIn("managers", meta.__dict__) | ||
|
|
||
| # Now expire the cache | ||
| meta._expire_cache(forward=True, reverse=False) |
There was a problem hiding this comment.
I wonder if we should just use the non-underscored apps.clear_cache(). In fact, we already have a test AppsTests.test_clear_cache calling that method. Given that, do you think we need a separate test module? What about just placing the new test adjacent to that one?
| self.assertNotIn("related_objects", meta.__dict__) | ||
|
|
||
| def test_model_inheritance_caching(self): | ||
| """Test that model inheritance properly handles property caching.""" |
There was a problem hiding this comment.
When docstrings communicate the same information as the test name (and the test name is sufficiently descriptive), we just chop them.
There was a problem hiding this comment.
Got it. Shall remove those unnecessary docstrings.
| class TestModel(models.Model): | ||
| name = models.CharField(max_length=100) | ||
|
|
||
| # Verify that none of the FORWARD_PROPERTIES are in the __dict__ initially |
There was a problem hiding this comment.
These are the kinds of details I don't think we need to spend effort testing (django.utils.cached_property itself is sufficiently tested)
There was a problem hiding this comment.
Sure. Still learning the code base. I will remove these tests
Trac ticket number
Ticket-36369
Branch description
What: This PR adds unit tests for FORWARD_PROPERTIES and REVERSE_PROPERTIES.
Why: Based on the discussions in this ticket and this #19245, it was obvious that unit tests were missing for FORWARD_PROPERTIES and REVERSE_PROPERTIES in models/options.py.
Main update: Introduced a new test file, test_meta_caching.py, containing unit tests focused on the _expiry_cache functionality in models/options.py, specifically targeting the use of REVERSE_PROPERTIES and FORWARD_PROPERTIES.
Checklist
mainbranch.