[Django] #21977: Deprecate SimpleTestCase.urls in favor of override_settings
Django
------------------------------------------------+------------------------
Reporter: timo | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
As summarized by Aymeric in #21518:
Clearly we'd better provide just "one way to [override `ROOT_URLCONF`" in
the long term. We could consider the "urls" attribute as an old and
specific way to override a particular setting. That would give us a reason
deprecate it in favor of the standard settings override mechanisms,
@override_settings(...) and with self.settings(...):.
--
Ticket URL: <https://code.djangoproject.com/ticket/21977>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: timo | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anubhav9042):
* cc: anubhav9042@… (added)
Comment:
Which one way we should be going towards?
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:1>
Django
Reporter: timo | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
We should see if it's feasible to replace `SimpleTestCase.urls = 'foo'`
(and deprecate this syntax) with `override_settings(ROOT_URLCONF='foo')`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:2>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anubhav9042):
* cc: anubhav9042@… (removed)
* status: new => assigned
* owner: nobody => anubhav9042
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:3>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anubhav9042):
* cc: anubhav9042@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:4>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anubhav9042):
Well I have tried replacing it at some places.
Is something like this:
[https://github.com/coder9042/django/compare/ticket_21977?expand=1]
required?
Also do we need to remove the {{{SimplaeTestCase.urls}}} feature
completely or just change usage??
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:5>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
`SimpleTestCase.urls` will need to raise a warning as described in
[https://docs.djangoproject.com/en/dev/internals/contributing/writing-code
/submitting-patches/#deprecating-a-feature Deprecating a feature]. You'll
need to wait until we cut the 1.7 branch as the deprecation will need to
start in 1.8.
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:6>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anubhav9042):
Ok I will work on the patch after 1.7 branch is cut out and new master is
there.
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:7>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anubhav9042):
I have some doubts regarding this.
- Should the function which sets `.urls` should be changed or we are just
changing its usage everywhere in the tests.
- Do I need to write a test for this?
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:8>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
Yes, the function which sets urls should warn with a
`RemovedInDjango20Warning` (two releases from now) so that users who are
using this feature are warned to update their code. The deprecation
message should say something like "SimpleTestCase.urls is deprecated, use
@override_settings(ROOT_URLCONF=...) instead.".
A test to verify that the warning is raised under the appropriate
circumstances wouldn't hurt.
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:9>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* cc: loic@… (added)
Comment:
I did a quick run with `-Wall` and found a couple of `SimpleTestCase.urls`
that needed updating. Running the test suite with `--selenium` found a
couple more, so there may be more skipped tests that need updating. I
guess the logs of the CI will have to be monitored on the various setups.
The `stacklevel` for this warning isn't helpful at all, I've tried levels
up to 6 with no luck, so the faulty `TestCase` really need to be specified
in the warning message. For some reason `-Werror` doesn't do anything
here, so the warning message is really the only indication one has to find
the guilty `TestCase`.
Testing the deprecation is not that easy, I came up with:
{{{
class DeprecatingSimpleTestCaseUrls(unittest.TestCase):
def test_deprecation(self):
class TempTestCase(SimpleTestCase):
urls = 'tests.urls'
def test(self):
pass
with warnings.catch_warnings(record=True) as recorded:
suite =
unittest.TestLoader().loadTestsFromTestCase(TempTestCase)
with open(os.devnull, 'w') as devnull:
unittest.TextTestRunner(stream=devnull,
verbosity=2).run(suite)
# FIXME: Check that the correct warning is raised.
self.assertTrue(recorded)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:10>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anubhav9042):
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/2517
Thanks Loic for help in testing and reviewing.
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:11>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"cd914e31c9a889f18c50c15b4f6ee4959624001f"]:
{{{
#!CommitTicketReference repository=""
revision="cd914e31c9a889f18c50c15b4f6ee4959624001f"
Fixed #21977 -- Deprecated SimpleTestCase.urls
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:12>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: closed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"1392aff440bc8be26ac09062265269a0414d96d1" 1392aff]:
{{{
#!CommitTicketReference repository=""
revision="1392aff440bc8be26ac09062265269a0414d96d1"
Refs #21977 -- Removed SimpleTestCase.urls per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:13>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: closed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by lanzz):
While this fix will clear the cache ''when the ROOT_URLCONF is
overridden'', it does nothing to clear them ''once that override goes out
of scope''. As a result once you override ROOT_URLCONF, that override
leaks out to all subsequent unit tests if they don't explicitly override
it too.
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:14>
Django
Reporter: timo | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: closed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Please open a new ticket with a failing test for Django's test suite or
steps to reproduce, thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/21977#comment:15>