Opened 4 months ago

Last modified 7 weeks ago

#36369 assigned Cleanup/optimization

Missing test coverage for FORWARD_PROPERTIES entries

Reported by: Clifford Gama Owned by: Senthil Kumar
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: FORWARD_PROPERTIES, Options
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

possibly also with REVERSE_PROPERTIES

With the exception of _forward_fields_map you can remove entries of field-related properties from FORWARD_PROPERTIES and see no test failures. Discovered during #36207 ​review

Change History (11)

comment:1 by Jacob Walls, 4 months ago

Triage Stage: Unreviewed → Accepted

I think this is worth covering, because at a glance it seems like a bug that db_returning_fields is omitted from the entries. A test would help clarify.

comment:2 by Senthil Kumar, 4 months ago

I'm new and would like to take this on. May I assign it to myself? Assuming it's not time-sensitive, I’ll aim to add the test cases over the next few days.

comment:3 by Jacob Walls, 4 months ago

Owner: set to Senthil Kumar
Status: new → assigned

Thanks for volunteering. There's some discussion in the linked PR review with Clifford's initial investigation, if that helps any.

comment:4 by Senthil Kumar, 4 months ago

Thanks Jacob. I will be creating a PR soon.

comment:5 by Senthil Kumar, 4 months ago

Has patch: set

comment:6 by Jacob Walls, 4 months ago

Patch needs improvement: set

comment:7 by Senthil Kumar, 3 months ago

Patch needs improvement: unset

comment:8 by Senthil Kumar, 3 months ago

Based on your suggestion, I’ve revised the tests to ensure that the correct caches are expired. As you mentioned above, I’ve also added a failing test for db_returning_fields.

I have a question: now that we have a failing test, should I go ahead and add db_returning_fields under FORWARD_PROPERTIES, or should I leave the test as-is and mark it as expected to fail? I’d appreciate your guidance on this.

comment:9 by Jacob Walls, 3 months ago

I left a note that it looks like additional properties besides db_returning_fields are worth clearing from the cache. Assuming we go with a subTest-style test instead of writing separate cases for each, I think fixing the behavior and testing in one commit would be simplest.

comment:10 by Jacob Walls, 3 months ago

Patch needs improvement: set

comment:11 by Senthil Kumar, 7 weeks ago

Patch needs improvement: unset

Thanks for the patience again. I have revised the unit tests based on the suggested flow.
I have created a new PR as the previous PR's commit history got messed up. I have also fixed the behaviour and all the tests are passing now.

Link to the new PR: ​https://github.com/django/django/pull/19645

Previous PR: ​https://github.com/django/django/pull/19484

I have also dropped a quick question about using apps.clear_cache() in the PR. Hope to get some clarification on that. Thanks

Last edited 7 weeks ago by Senthil Kumar (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top