Opened 11 years ago

Closed 11 years ago

Last modified 8 years ago

#21977 closed Cleanup/optimization (fixed)

Deprecate SimpleTestCase.urls in favor of override_settings

Reported by: Tim Graham Owned by: ANUBHAV JOSHI
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: anubhav9042@…, loic@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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(...):.

Change History (15)

comment:1 by ANUBHAV JOSHI, 11 years ago

Cc: anubhav9042@… added

Which one way we should be going towards?

comment:2 by Tim Graham, 11 years ago

We should see if it's feasible to replace SimpleTestCase.urls = 'foo' (and deprecate this syntax) with override_settings(ROOT_URLCONF='foo').

comment:3 by ANUBHAV JOSHI, 11 years ago

Cc: anubhav9042@… removed
Owner: changed from nobody to ANUBHAV JOSHI
Status: newassigned

comment:4 by ANUBHAV JOSHI, 11 years ago

Cc: anubhav9042@… added

comment:5 by ANUBHAV JOSHI, 11 years ago

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??

comment:6 by Tim Graham, 11 years ago

SimpleTestCase.urls will need to raise a warning as described in 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.

comment:7 by ANUBHAV JOSHI, 11 years ago

Ok I will work on the patch after 1.7 branch is cut out and new master is there.

comment:8 by ANUBHAV JOSHI, 11 years ago

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?

comment:9 by Tim Graham, 11 years ago

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.

comment:10 by loic84, 11 years ago

Cc: loic@… added

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)

comment:11 by ANUBHAV JOSHI, 11 years ago

Has patch: set

PR: https://github.com/django/django/pull/2517

Thanks Loic for help in testing and reviewing.

comment:12 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In cd914e31c9a889f18c50c15b4f6ee4959624001f:

Fixed #21977 -- Deprecated SimpleTestCase.urls

comment:13 by Tim Graham <timograham@…>, 9 years ago

In 1392aff:

Refs #21977 -- Removed SimpleTestCase.urls per deprecation timeline.

comment:14 by Mihail Milushev, 8 years ago

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.

Actually on a second look it seems it might intend to do that too, but I am seeing leaking overrides in my tests, will investigate it more.

Version 1, edited 8 years ago by Mihail Milushev (previous) (next) (diff)

comment:15 by Tim Graham, 8 years ago

Please open a new ticket with a failing test for Django's test suite or steps to reproduce, thanks.

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