Fixed #35666 -- Documented stacklevel testing and adjusted test suite accordingly.

[charettes]

Trac ticket number

ticket-35666

Branch description

Per a discussion #18463 I figured I'd spend some time auditing all our usage of warnings.warn to see if we had more violations. I addressed them and adjusting the documentation about deprecating a feature to mention that filename should be compared to __file__.

[charettes]

Small note that just like #18463 the first three commits of this PR should likely also be backported to 5.1.x.

[jacobtylerwalls]

(Small typo in the first commit message: move -> model)

[jacobtylerwalls]

Sorry to be a pest, but there are two more typos: Movel -> Model as well as Adusted -> Adjusted (d20f90d)

[charettes]

@jacobtylerwalls not a pest at all, thanks for letting me now!

[charettes]

@nessita the warnings that were not adjusted in this PR were not covered because there doesn't exist a literal stacklevel to surface the error at the right location.

model_forms/tests.py

The warning is emitted from deep inside settings code. It only makes sense to emit the warning in the context of project setup (it's triggered by the test because of the usage of the settings context manager). We want this warning to let the user know that a setting is deprecated and it's not tied to a particular location in the current call stack.

urlpatterns/tests.py one_to_one/tests.py:615 modeladmin/tests.py:969

I missed these ones, they have now been adjusted thanks!

utils_tests/test_html.py:74

This one is covered by the referenced PR by @adamchainz that led to this systemic cleanup. I kept things separately to make sure he gets attribution.

modeladmin/tests.py:300

This one I feel like the warning is implicit enough about what should be done and I'm not sure that trying to guess the proper call site is appropriate. get_filters could be called from different locations and truncating the stack might be more confusing than helpful.

foreign_object/tests.py:745 (test_join_get_joining_columns_warning):

This one is a no-no IMO. It might work in the context of this particular test but compilation can be triggered from multiple different locations. In other words, just like get_filters there are potentially different callstacks with different public entry points.

Also, I would suggest that we add a small release note for 5.1.1 mentioning the fix for this release blocker.

I'll do that in a distinct commit. Feel free to move it around.

[nessita]

model_forms/tests.py

The warning is emitted from deep inside settings code. It only makes sense to emit the warning in the context of project setup (it's triggered by the test because of the usage of the settings context manager). We want this warning to let the user know that a setting is deprecated and it's not tied to a particular location in the current call stack.

Makes sense, thanks for the details.

utils_tests/test_html.py:74

This one is covered by the referenced PR by @adamchainz that led to this systemic cleanup. I kept things separately to make sure he gets attribution.

Makes sense, and I just merged #18463.

modeladmin/tests.py:300

This one I feel like the warning is implicit enough about what should be done and I'm not sure that trying to guess the proper call site is appropriate. get_filters could be called from different locations and truncating the stack might be more confusing than helpful.

Fair enough! I did spot one more that we missed in this file, for test_log_deletion. I'll be adding it to your "Fixed #35666..." commit.

foreign_object/tests.py:745 (test_join_get_joining_columns_warning):

This one is a no-no IMO. It might work in the context of this particular test but compilation can be triggered from multiple different locations. In other words, just like get_filters there are potentially different callstacks with different public entry points.

Also makes sense, thanks for the clarification.

Also, I would suggest that we add a small release note for 5.1.1 mentioning the fix for this release blocker.

I'll do that in a distinct commit. Feel free to move it around.

Thanks, will do! Likely will end up with the "Fixed #35666..." commit since that is "the" commit that fixes the release blocker.

Thank you so much for your work and help! :star2:

[nessita]

@charettes I have adjusted the commit messages a bit and replaced a ticket reference which I think was a typo: for the staticfiles finders, it was reported to Refs #35326 but I'm pretty sure it should be Refs #22712.

Also, after a detailed inspection, I now see what you meant about being unclear where to add the release note, since the "Fixed" commit is not really changing any stacklevel, but only adding test assertions. Therefore, I think one other option would be to write specific notes for each commit that needs backporting. I'll work something out for this (tomorrow most likely).

[nessita]

For the sake of explicitness, commits referencing ticket-35326, ticket-35405, and ticket-35060 will be backported to 5.1 with a dedicated release note. The commit fixing ticket-35666 will be also be backported, but without a release note.

[nessita]

I believe this is ready to be merged (once CI is green again). I have:

• sorted the commits by ticket number
• added release notes for the three issues mentioned above
• removed release note from the last commit
• fought interactive rebase for a while, wishing there would be a way to do "rebase inception" (i.e. rebase a subset a commits when in the middle of a bigger interactive rebase) :rofl:

[adamchainz]

Well, thank you for finishing it off! You're a ⭐