Code

Opened 4 years ago

Closed 3 years ago

Last modified 4 months ago

#14319 closed New feature (wontfix)

Add signals test_setup and test_teardown to Django test suite runner Options

Reported by: jsdalton Owned by: nobody
Component: Testing framework Version: master
Severity: Normal Keywords: signals
Cc: adam@…, jsdalton Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I would like to submit a proposal (with patch) for two new signals to add to the testing framework: a test_setup signal, to be emitted during global test setup, and a test_teardown signal, to be emitted during global test teardown.

Presently, there is no way to modify or add to the global setup or teardown behavior that runs with the built-in test suite runner. This patch provides a hook that applications can use to do that. Each signal is emitted once during a test run.

The proposal was discussed briefly on the Django developer's list here:
http://groups.google.com/group/django-developers/browse_thread/thread/f45c7302634ed82d

Here are a few notes about the proposed changes to source code:

  • There are really only a handful of lines of code required to make this change.
  • The most significant change is that the ordering of actions in the run_tests() method of DjangoTestSuiteRunner has been adjusted. In order for applications to be able to setup connect() calls to the signals, the application code itself has to be imported. This happens for the first time in build_suite(), so I swapped the order of the call to build_suite() and the call to setup_test_environment(). This appears to have no impact, but it's conceivable that some issues could arise if e.g. an application developer did something during import that they expected to be disabled first during testing. At present, the main thing Django does in the setup_test_enivronment() call is to disable the email backend, so unless a developer is sending emails in the body of a module or something it shouldn't present a problem.
  • The second most significant change is that the django.test.utils functions setup_test_environment() and tear_down_environment() calls are executed by connecting to the signal rather than being called directly from the methods of the test runner. This serves as a way to test that the signals are being fired, as Django tests will fail if those two functions do not execute. (Thanks to Russell Keith-Magee for this suggestion).
  • I have included documentation for these two new signals on the Django signals documentation page.

Here are a few arguments in favor of this proposal that I posted to the Django developers discussions:

  • Requiring a custom test runner to implement this behavior makes it (nearly) impossible for reusable applications to modify global setup and teardown behavior, since it would become the responsibility of the project itself to specify the custom test runner.
  • The current setup gives the Django core "privileged" access to disable certain features during testing, it would seem that application should be given the capability as well.
  • Signals are non obtrusive...if they are not used they don't really harm anything.
  • None of the proposed changes would impact production code, since they are all restricted to the test suite. In fact the patch is really only about a few lines of additional code.

There maybe other arguments in favor of or against as well, please feel free to share them if so.

Attachments (3)

new_test_signals_patch.diff (3.8 KB) - added by jsdalton 4 years ago.
Patch to add test_setup and test_teardown signals to Django test framework
new_test_signals_patch_2.diff (3.7 KB) - added by aseering 3 years ago.
Updated patch, applies against trunk as of r16181
new_test_signals_patch_3.diff (4.2 KB) - added by jsdalton 3 years ago.

Download all attachments as: .zip

Change History (16)

Changed 4 years ago by jsdalton

Patch to add test_setup and test_teardown signals to Django test framework

comment:1 Changed 4 years ago by ericholscher

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by graham_king

  • Keywords signals added
  • Severity set to Normal
  • Type set to New feature

comment:3 Changed 3 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

new_test_signals_patch.diff fails to apply cleanly on to trunk

Changed 3 years ago by aseering

Updated patch, applies against trunk as of r16181

comment:4 Changed 3 years ago by aseering

  • Cc adam@… added

I'd like to have this feature as well. I've attached an updated patch that applies against trunk. (Only trivial changes from the original patch.)

The documentation with this patch still refers to its becoming available in Django 1.3. I assume that should be updated, but I'm not sure what to update it to?

comment:5 Changed 3 years ago by jsdalton

  • Cc jsdalton added
  • milestone changed from 1.3 to 1.4
  • Patch needs improvement unset

Thanks @aseering. Looks like there were a few additional issues applying your patch against trunk, so I cleaned those up. I also bumped the version to 1.4 in the docs.

This now applies cleanly against r16277 and all tests pass. Hopefully this can now be marked as ready for checkin by a reviewer.

Changed 3 years ago by jsdalton

comment:6 follow-up: Changed 3 years ago by ptone

  • Patch needs improvement set
  • UI/UX unset

It is unclear why the patch is removing the global setup and tear down calls setup_test_environment and teardown_test_environment

comment:7 in reply to: ↑ 6 Changed 3 years ago by jsdalton

  • Patch needs improvement unset

Replying to ptone:

It is unclear why the patch is removing the global setup and tear down calls setup_test_environment and teardown_test_environment

I mentioned this up in the ticket description:

"The second most significant change is that the django.test.utils functions setup_test_environment() and tear_down_environment() calls are executed by connecting to the signal rather than being called directly from the methods of the test runner. This serves as a way to test that the signals are being fired, as Django tests will fail if those two functions do not execute. (Thanks to Russell Keith-Magee for this suggestion)."

Basically those functions are hooked to the new global setup/teardown signals here rather than being called directly. Makes sense?

comment:8 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:9 Changed 3 years ago by ptone

  • Patch needs improvement set

OK, I see - there is an import that needs to be deleted, I think that might have been what was throwing me off.

Also, even though the preceding item mentions it, I think it is redundant to say that this signal does not fire during normal operation in each test description, since that caveat is introduced in the test signals section.

Another possible heads up that came from IRC, is that there is some discussion about abandoning the Django custom test harness in place of a standardized one based on nose or test.py, and so there is some concern about adding features to the current harness

comment:10 Changed 3 years ago by Alex

  • Resolution set to wontfix
  • Status changed from new to closed

After a discussion with Carl yesterday I'm marking this as wontfix. This idea continues a fundamentally bad trend in Django: treating it as some sort of application runner, rather than a library. To that end, our long running goal should be to kill our test harness, and recreate the database setup stuff as plugins for unittest2 (which means it's then runnable by other test framworks like nose or py.test), once we're firmly in that territory users will have the same things available to them: they can write unittest plugins to do their own test suite setup.

comment:11 Changed 3 years ago by jsdalton

What you're saying sounds reasonable but it's hard for me to really understand the context in which this decision has been made. Is there a ticket or discussion that relates to this idea of moving the Django test suite toward a "plugin" approach? I googled plugins for unittest2 but I couldn't really find anything about it either.

I'd be happy to assist in any efforts that are being made to move the testing suite in Django in the direction you are suggesting, but I'm concerned that we're killing a useful improvement for the sake of future ambitions that might never materialize.

comment:12 follow-up: Changed 4 months ago by mrmachine

Has any progress been made towards recreating the database setup stuff as plugins for unittest2? Or is there a ticket to track progress on that front?

I know that Django 1.6 shipped with a new test runner allowing improved and more consistent test discovery, but that doesn't seem to be a step in the direction of making Django less of an application runner and more of a library.

In fact, Django 1.7 will ship with the app-loading refactor which promises "applications can run code at startup, before Django does anything else, with the read() method of their configuration".

I could be wrong, but doesn't that seem to indicate that, bad trend or not, treating Django as some sort of application runner is here to stay. If so, and especially if there are no concrete plans to remove the test runner and replace it with unittest2 plugins for database setup as described above, it would be good to have startup hooks in the test runner as provided by this ticket.

For my own use case, I have a custom management command that performs some initial application setup and I need to run before any tests, and would like to avoid running it in every TestCase.setUp() method only to be rolled back and re-run for the next test.

comment:13 in reply to: ↑ 12 Changed 4 months ago by russellm

Replying to mrmachine:

Has any progress been made towards recreating the database setup stuff as plugins for unittest2? Or is there a ticket to track progress on that front?

Well, I'm not even sure what you mean by this, so it's probably safe to say there isn't a ticket.

I know that Django 1.6 shipped with a new test runner allowing improved and more consistent test discovery, but that doesn't seem to be a step in the direction of making Django less of an application runner and more of a library.

In fact, Django 1.7 will ship with the app-loading refactor which promises "applications can run code at startup, before Django does anything else, with the read() method of their configuration".

I could be wrong, but doesn't that seem to indicate that, bad trend or not, treating Django as some sort of application runner is here to stay. If so, and especially if there are no concrete plans to remove the test runner and replace it with unittest2 plugins for database setup as described above, it would be good to have startup hooks in the test runner as provided by this ticket.

I'm not sure how you've come to this conclusion. Django isn't transforming into a "runner". It's providing a library-level mechanism that apps can tie into so that they can register "once at startup" logic. There is no master "Django" process; it's still up to the wsgi container (or whatever is running the Django stack) to configure the Django library; making sure all the apps are configured are now part of that remit.

For my own use case, I have a custom management command that performs some initial application setup and I need to run before any tests, and would like to avoid running it in every TestCase.setUp() method only to be rolled back and re-run for the next test.

Sure - I can see the value in this; but I'm not sure why it isn't possible with an extension of the test runner. There might be room to make the interface cleaner, but AFAICT it's *possible* right now.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.