#2879 closed New feature (fixed)
Add live test server support to test framework
Reported by: | Owned by: | devin | |
---|---|---|---|
Component: | Testing framework | Version: | |
Severity: | Normal | Keywords: | |
Cc: | Russell Keith-Magee, allandouglas@…, dnaquin@…, Johannes Beigel, andrew@…, Malcolm Tredinnick, Éric St-Jean, martin@…, simon@…, alex@…, zodizz@…, robert.ramirez@…, gabor@…, clouserw@…, Goldan, phartig@…, roman@…, dwight@…, lrekucki@…, kmike84@…, ShawnMilo, bradley.ayers@…, tom@…, Francis Devereux, hjwp2@…, anssi.kaariainen@…, unbracketed, jian@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This was necessary for ticket:2867 but is useful enough on it's own to be a separate enhancement ticket.
Added module is django.test.http which contains all the code to bring up a slightly modified version of the regular django test server. The module is simple to use, you simply create a unit test that inherits from django.test.http.HttpTestCase. That class contains setUp and tearDown methods that bring up the test server and kill it when the test is complete.
The django test server had huge issues with the in memory database, which was the test framework default. I brought this up on the dev list and saw that other had issues using the in memory database as well. I removed ':memory' from being the default database, although if TEST_DATABASE_NAME is set to ":memory:" it will still work. I modified the database cleanup code to account for both cases as well (Note: previously the cleanup code didn't account properly for regular sqlite databases, it now accounts for both).
Attachments (27)
Change History (129)
by , 18 years ago
Attachment: | http_test_case.diff added |
---|
comment:1 by , 18 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Summary: | Add live test server support to test framework → [patch] Add live test server support to test framework |
Adding patch to subject.
by , 18 years ago
Attachment: | http_test_case.2.diff added |
---|
fixed comment typo and improper print statement
comment:3 by , 18 years ago
Owner: | removed |
---|
Un-assigning from myself since this needs to be reviewed by a commiter.
comment:4 by , 18 years ago
Owner: | set to |
---|
comment:5 by , 18 years ago
How does this relate to the unit test system that has evolved since then?
comment:6 by , 18 years ago
I haven't looked at the code base since I wrote this patch. So my short answer would be that it doesn't relate at all and probably breaks.
If the new unittest system includes support for running a live server and making real requests then this patch is obsolete.
If the new unittest system doesn't have list server support then I should probably rewrite this patch to provide the support within the new framework.
comment:7 by , 18 years ago
Wow, Mikeal, that was a fast reply. Could you, as time permits, look whether this is still needed and put a little comment in the ticket?
comment:8 by , 18 years ago
Cc: | added |
---|
Or perhaps Russell could take a short look, he should be able to spot it without looking ;-) I don't know the unit test so well. I'm only doing triage here.
comment:9 by , 18 years ago
The testing documentation doesn't seem to have changed since I wrote this patch.
http://www.djangoproject.com/documentation/testing/
Regardless I think I could do this a little more elegantly now. I also read about support for cherrypy's wsgi when installed which I think would be important to add since that big hack to kill the server isn't necessary using the cherrypy server. Eww, and I'm raising a string instead of a proper exception in there.
I'll try to find some time this week or next to work on this.
comment:10 by , 18 years ago
With the core-management.patch it would be possible to start a test server like './manage.py runserver' with the command './manage.py runtestserver'. It would be setup a test-db like with './manage test' and loaded fixtures called 'initial_data' and 'test_data'.
comment:11 by , 18 years ago
Together with the core-management.patch it would be possible to flush a running live test server between tests.
comment:12 by , 18 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:13 by , 18 years ago
Confirmed that it worksforme on SVN HEAD of unicode branch.
Just, changes made on django/test/utils.py by http_test_case.2.diff patch has to be reverted, as database syssetm changed meantime.
I'd be very very glad to see this patch being checked in ASAP as I'm using it on all my projects already.
by , 18 years ago
Attachment: | test-server-support.patch added |
---|
All patches in one, with reverted utils.py
comment:14 by , 17 years ago
Cc: | added |
---|
comment:15 by , 17 years ago
Triage Stage: | Design decision needed → Accepted |
---|
comment:16 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I believe this was fixed in [5912] with the addition of manage.py testserver
.
comment:17 by , 17 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
This is a little different to manage.py testserver
. The testserver command is about getting a server that uses test data; this ticket is about starting a server during a test case so that a test case can poke AJAX-style methods, or write a view that in turn makes HTTP calls. This is needed if you are going to run Selenium-style tests, as the test case needs to interact with a live running server.
comment:18 by , 17 years ago
Is there a way to see this one in trunk?
Anything needs to be done, or any way how to speed it up?
follow-up: 21 comment:19 by , 17 years ago
The patch attached to this bug won't work in the current trunk.
I can write another one but after seeing what happened to the last patch I'd like to know that someone is actually going to review it before it becomes outdated like the last one.
comment:20 by , 17 years ago
Sorry, i was trying to apply the wrong patch.
Haven't tried the newest set.
comment:21 by , 17 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Replying to Mikeal Rogers <mikeal@osafoundation.org>:
The patch attached to this bug won't work in the current trunk.
I can write another one but after seeing what happened to the last patch I'd like to know that someone is actually going to review it before it becomes outdated like the last one.
The requirements for this patch are the same as every other patch, as documented here:
http://www.djangoproject.com/documentation/contributing/#patch-style
In short:
- Your patch needs to encompass a single idea. In this case, adding a TestCase with a test server. The :memory: change is probably a reasonable change, but is unrelated to the ticket purpose. It should be logged and processed as separate issue.
- Your patch needs tests. A regression test that demonstrates that the new test case works is an absolute minimum. Unless a patch is trivial, or I need the feature/fix myself, or it's difficult/impossible to test the feature (e.g., testing command line options to ./manage.py), I won't even bother looking at a patch until it has tests. I'm too busy to spend my days trying to work out how other peoples code should work. A test is how you prove you are serious about using my time.
- Your patch needs documentation. If you have tests, documentation can wait until after an initial review - but if you don't have a test, then you're going to need to document how your feature works.
- (Generally required, but not so much in this case since this ticket is on my watch list) - the triage team needs to review and promote the patch.
follow-up: 23 comment:22 by , 17 years ago
I originally wrote this a long time ago (at least in web years). I've learned a lot since then and have mde my way through a dozen test frameworks and developed a few myself. I took a few hours out of my day today and try to re-assess the situation.
IMHO TestCase is not the right place for this. TestCase can only define setup and teardown for a single unittest class, this needs to define something that can be a product of the test environment that encompasses multiple test classes and suites.
It's been a while since I monkeyed around with unittest, but having done this in the past I know it's not easy and is one of the primary reasons people don't like unittest.
I also haven't looked at the test infrastructure for django in a long time, just a quick glance shows that a lot seems to have changed, so I don't know if this is even possible.
Eventually I'm going to need to integrate windmill (http://windmill.osafoundation.org) tests in to my own django project. When that work is finished I'll send a proposal to the list to see if it would be a valued contribution, but doing this right will probably require a new collector based framework with more acutely defined setup/teardown.
In the meantime, if anyone wants to take on this work again ( adding a TestCase with live server support ) they are more than welcome to it, you can email me personally if you have any implementation specific questions. I'm more than happy to help I'm just not up for wrangling unittest anymore.
comment:23 by , 17 years ago
Replying to Mikeal Rogers <mikeal@osafoundation.org>:
I'm more than happy to help I'm just not up for wrangling unittest anymore.
Well, it's not a problem to implement nose et al via settings.TEST_RUNNER option. However, even if SeleniumTestCase (or equivalent) is discovered, connected to selenium proxy and browser is started successfully, tests are still not usable as they cannot connect to live server :]
(IMHO biggest flaw in unittest is absence of SkipTest...)
by , 17 years ago
Attachment: | run_test_server_simple.py.diff added |
---|
comment:24 by , 17 years ago
Well, the last patch seems not to work in current trunk version. The structure of django/core has changed, and I see no obvious way of implementing the patch.
I've created my own patch which seems to do the job. At least it works for me. Probably you'll need Python 2.5 to run it and, of course, Linux (since the fork() method is not implemented in Windows, as I believe). I tested it only with SQLite and really don't know if it'll work with other backends.
The patch does a simple thing: after initializing environment and creating a test database, it forks and in child process a Django server is started. Being a child, it inherits the environment. In parent process, a test is started (which can include both Django TestCase and Selenium methods), and when it finishes the child is killed and the parent returns the result. The patch consists of two files, one for django/test/simple.py and another for django/test/utils.py.
One little thing to mention: I couldn't manage to make this idea work when I used :memory: database (which is default for SQLite in tests), thus I changed it to file. Then it worked.
Using this method you probably will not have to use testserver (see [5912]).
Any comments would be much appreciated.
comment:25 by , 17 years ago
Sorry, but I can't figure out why diff for the first file isn't displayed.
comment:26 by , 17 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
I have something working that will add this functionality to TestCase. I'll clean up my patch, write some tests, and get something here today or tomorrow.
comment:27 by , 17 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
I've added this functionality to the TestCase class.
The problem Mikeal mentioned with regards to in-memory databases (http://tinyurl.com/5p3g2y) was not a problem with the database, only in the attempt to access an in-memory database within a separate thread. Because of the threading, we have to special case in-memory databases.
Tests and documentation changes are included.
comment:28 by , 17 years ago
Cc: | added |
---|
comment:29 by , 17 years ago
Could somebody clarify for me what this does that can't be handled by, or built on top of, django-admin.py/manage.py testserver
? Since that's implemented as a management command, it can be called from Python code as needed, which (if this feature is still needed to address a problem with testserver
) looks like it'd greatly simplify this patch.
comment:30 by , 17 years ago
Yeah, directly calling testserver would greatly simplify things. The main problem with that, however, is that testserver isn't stoppable. After running a test, I need to be able to stop the server, reload fixtures, etc.
I agree though, I'm not happy with how much code I'm repeating. Suggestions welcome.
comment:31 by , 17 years ago
Cc: | added |
---|
comment:32 by , 17 years ago
Cc: | added |
---|
comment:33 by , 17 years ago
Patch needs improvement: | set |
---|
I'm working on improving the way this fails when problems starting a test server are encountered. I'll have a better patch shortly.
by , 17 years ago
Attachment: | django_live_server.diff added |
---|
fixes way server handles error on startup
comment:34 by , 17 years ago
Patch needs improvement: | unset |
---|
comment:35 by , 16 years ago
Cc: | added |
---|
comment:36 by , 16 years ago
Cc: | added |
---|
comment:37 by , 16 years ago
Cc: | added |
---|
comment:38 by , 16 years ago
One thing to note about my patch, while it's on my mind:
In order to be able to stop the running test webserver, I need to set a socket timeout on waiting for requests. eg:
def server_bind(self): """ Sets timeout to 1 second. """ WSGIServer.server_bind(self) self.socket.settimeout(1)
But what happens when you're waiting on a request that always takes longer than 1 second? You'll continually timeout? You'll fail on a selenium wait? Decidedly something quote unquote not good.
I don't know a clean solution to this. I need some kind of timeout. Otherwise, there's no way to stop mid handle_request(). And then no way to kill the server after a test is done. It'll just hang on that handle_request(). But any timeout is subject to problems if it needs to handle a request that's going to take longer.
# Loop until we get a stop event while not self._stopevent.isSet(): httpd.handle_request()
And since there's no way to kill a python thread, there's no other way to stop this server.
We could have this timeout as a parameter and default to 1s. That way at least if people run up against the problem they can up the timeout and fix it.
comment:39 by , 16 years ago
Nevermind. The patch handles this.
That server_bind timeout is just for receiving requests. Once the server grabs a request it sets the socket for handling it to None.
sock, address = self.socket.accept() sock.settimeout(None) return (sock, address)
So requests can take however long.
Actually. The timeout should be a lot smaller then. 0.001s working fine for me.
follow-up: 43 comment:40 by , 16 years ago
Cc: | added |
---|
comment:41 by , 16 years ago
Cc: | added |
---|
by , 16 years ago
Attachment: | django_live_server_r8458.diff added |
---|
Updated devin's patch with last changes in testing environment
follow-up: 45 comment:42 by , 16 years ago
I borrowed the code from the latest patch here to add Django support to windmill.
http://trac.getwindmill.com/browser/trunk/windmill/authoring/djangotest.py
It hasn't been fully documented yet but the simplest case was written up for the email list.
http://groups.google.com/group/windmill-dev/browse_thread/thread/85f23d2a0d4e99
comment:43 by , 16 years ago
comment:44 by , 16 years ago
Cc: | added |
---|
comment:45 by , 16 years ago
Replying to mikeal:
I borrowed the code from the latest patch here to add Django support to windmill.
I borrowed the code and approach too and added it as nose plugin, so it's usable for usual test cases too. It's part of django-sane-testing package, you can subclass from HttpTestCase or add start_live_server attribute to you testcase class and server should be available.
comment:46 by , 15 years ago
Cc: | added |
---|
comment:47 by , 15 years ago
any progress or changes to work on 1.1.1? I don't seem to find it anywhere
comment:48 by , 15 years ago
Cc: | added |
---|
comment:50 by , 15 years ago
Cc: | added |
---|
comment:51 by , 14 years ago
Cc: | added |
---|
comment:52 by , 14 years ago
Cc: | added |
---|
comment:53 by , 14 years ago
Severity: | normal → Normal |
---|---|
Type: | enhancement → New feature |
comment:54 by , 14 years ago
Cc: | added |
---|---|
Summary: | [patch] Add live test server support to test framework → Add live test server support to test framework |
I just found the need for this feature, so adding myself to CC. I'll try to reimplement it using unittest2, so it can benefit from class/module level fixtures.
comment:55 by , 14 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
comment:56 by , 13 years ago
Cc: | added |
---|---|
UI/UX: | unset |
comment:57 by , 13 years ago
Thank you all for the precious early work on this. I've used the initial patches here and tried to come up with a clean implementation and a simple API (see the attached patch). I've also written a few Selenium tests for the admin; many more admin tests could be written but I think for now that's enough to show that it works.
Several things could probably be improved. Perhaps the Selenium and live servers should be launched in setUpClass()
instead of setUp()
. There also probably are some edge cases that aren't covered yet, as so far I've only tested with sqlite on Chrome and Firefox.
Finally, one particular comment on threading. As currently implemented, the live server continuously waits for requests until the please_stop
event is set. Since the socket is blocked until the next request is received, I've made it to send on last dummy request to finally unblock and close the server. Although I could live with that, it'd be nice to do it a bit differently. Some good clues can be found there: http://www.velocityreviews.com/forums/t675145-stopping-socketserver-on-python-2-5-a.html
I've written some doc which should hopefully be enough to get you started. Any feedback would be greatly appreciated. Thanks!
by , 13 years ago
Attachment: | 2879.selenium-support.diff added |
---|
comment:58 by , 13 years ago
To save you some time, here's how to run the few admin tests that have been written so far:
./runtests.py --settings=test_sqlite admin_widgets.AdminWidgetsSeleniumTests ./runtests.py --settings=test_sqlite admin_inlines.AdminInlinesSeleniumTests.test_add_inlines ./runtests.py --settings=test_sqlite admin_inlines.AdminInlinesSeleniumTests.test_delete_inlines
comment:59 by , 13 years ago
Cc: | added |
---|
by , 13 years ago
Attachment: | 2879.selenium-support.2.diff added |
---|
comment:60 by , 13 years ago
The latest patch brings two major improvements: 1) static media are now handled properly, allowing Selenium tests to be run from a project via manage.py test
(the previous patch only allowed them to work in the Django test suite); and 2) I think I've solved the threading issues by borrowing a bit of code from SocketServer.BaseServer
in Python 2.6.
I'm now reasonably happy with the implementation and will move on to writing more documentation. At this stage I'd really appreciate getting some feedback from testers so we can identify and address any edge cases. Thank you!
by , 13 years ago
Attachment: | 2879.selenium-support.4.diff added |
---|
comment:61 by , 13 years ago
The patch now comes with full documentation and is candidate for RFC. Any feedback welcome. Thanks!
comment:62 by , 13 years ago
Read through the patch—it looks sensible and well documented, and includes a few example tests to ease authoring of frontend tests in the admin.
I'll pull the patch tonight and play with it, but on the face of it, looks fantastic. Nothing missing. Will be very exciting to have this in Django. +1!
comment:63 by , 13 years ago
Cc: | added |
---|
comment:64 by , 13 years ago
Cc: | added |
---|
comment:65 by , 13 years ago
+1, great feature - looked through the patch, everything makes sense.
I would note that it's aimed at the "RC" flavour of selenium. There's another flavour called "WebDriver" which is slightly less complex, and for example doesn't require the .jar proxy server to be running.
It looks like it would be reasonably easy to support WebDriver in a future version - it would be an alternative subclass of LiveServerTestCase, perhaps called SeleniumWebDriverTestCase, which would suggest renaming SeleniumTestCase to SeleniumRCTestCase.
But I don't think this needs to affect the release of the patch as it is... If I get some time I could have a crack at writing some additions, but I don't want to delay this being released as it is...
comment:66 by , 13 years ago
Cc: | added |
---|
comment:67 by , 13 years ago
@hjwp2 The Selenium-specific code in this patch is quite minimal and indeed it only concerns the Remote Control (RC) flavour. Most of the patch's code is for LiveServerTestCase
, which makes it easy to plug any other functional testing frameworks like Selenium or Windmill and unit testing frameworks like QUnit. Specific implementations for supporting those frameworks should be done outside of Django itself and in third-party packages. However, it's necessary for Django to ship full support for at least *one* of them, so that tests can be written for its own code (e.g. admin, auth, comments). So far I've picked Selenium RC and wrote a few tests for the admin as a proof of concept. It seems like WebDriver is currently not as fully-featured as RC but it's catching up and it's going to be the future of Selenium. So it would probably be wiser to pick it now as a longer-term solution. I'll see if the tests I wrote can easily be converted. If you've got some experience with WebDriver then please feel free to chime in.
comment:68 by , 13 years ago
Some thoughts:
- Given that the front page of http://seleniumhq.org/ now reads "Selenium WebDriver is the successor of Selenium Remote Control which has been officially deprecated." I think we need to either support both RC and WebDriver, or only WebDriver. I'd be fine with supporting both, as the pragmatic approach, but it'd certainly be worth renaming
SeleniumTestCase
toSeleniumRCTestCase
. Supporting WebDriver could then be a separate ticket. - I'm not sure it's okay to attempt making an HTTP connection whenever importing django.test.testcases. Waiting until the first test is run would probably be better https://gist.github.com/1372740.
- Would it be worth considering moving
SeleniumTestCase
(orSeleniumRCTestCase
andSeleniumWebDriverTestCase
) into a new packagedjango.contrib.selenium
? (ObviouslyLiveServerTest
would stay indjango.test
)
comment:69 by , 13 years ago
Attached patch does the following:
- Rename
SeleniumTestCase
toSeleniumRCTestCase
, settings toSELENIUM_RC_HOST
,SELENIUM_RC_PORT
,SELENIUM_RC_BROWSER
, and updates docs accordingly. - Move
SeleniumTestCase
intodjango.contrib.selenium
. - Check for Selenium connection during the first test run, rather than at time of import.
- Move "Wait for the Django server to be ready" code out of
SeleniumTestCase
, intoLiveServerTestCase
.
Feel free to pull out whatever you think is useful and chuck out the rest.
I've take a look at using WebDriver instead - it seems pretty complete to me, it's much nicer not having to have the Selenium Server running, and it looks a lot faster to setup/teardown. (A sample test doing a single URL load tended to complete in 2.0-2.5 seconds, compared to 5.0-6.0 seconds for the RC version). The API is perhaps a little more verbose than it could be, but there's nothing obviously preventing the existing admin tests from being re-written for WebDriver.
by , 13 years ago
Attachment: | 2879.selenium-support.5.diff added |
---|
comment:70 by , 13 years ago
Thank you Tom, all your changes make sense. It just seems that you forgot to include the new files from contrib/selenium/, no?
I'm not sure we should create a new contrib.selenium
app, though. The future trend is going to gradually split the existing contrib apps out of core, so adding a new one now is likely not going to fly. The most important thing is to provide a generic and stable API, which is already addressed by LiveServerTestCase
. I think it's ok to provide full support for *one* flavour of Selenium (or another framework) in django.test
so that the current contrib apps can be tested. I'll bring this particular question up on #django-dev to hear other core devs' point of view on this.
Also, if we end up using solely WebDriver
, then SeleniumRCTestCase
should probably be split out to a third party app to reduce the maintenance responsibility on Django itself.
In the meantime, the next step is to try to convert the existing admin tests to using WebDriver
. I'll try to have a go at it in the coming days.
comment:71 by , 13 years ago
One idea that's been brought up on IRC is to keep the minimal amount of code necessary to run the admin tests (i.e. SeleniumTestCase
) in contrib.admin.tests._selenium
, or something like that. The idea is to not create another contrib app, and to not bless Selenium as Django's official in-browser testing framework.
LiveServerTestCase
would obviously remain in core (in django.test
) to allow third-party apps to provide support for Selenium, Windmill, etc.
As far as the documentation is concerned, some notes would be needed in the contributing docs so that people know how to run the admin Selenium tests. For the general case, a tutorial on how to write Selenium/Windmill/etc. tests for a Django app or project could also be considered for inclusion.
comment:72 by , 13 years ago
It just seems that you forgot to include the new files from contrib/selenium/, no?
Oops. Added now :) - And "git add -N" now learned!
comment:73 by , 13 years ago
WebDriver is much nicer to use than the remote driver. Partly because of the improved api and partly because of the removal of the need to run the selenium jar.
Selenium is substantially the most popular browser-driving test tool, so I don't see a problem in "blessing it" by providing built-in support.
comment:74 by , 13 years ago
I've converted the tests to using WebDriver in Firefox (tested) and IE (untested). Would anyone with a Windows environment be keen to try it out? :)
WebDriver indeed has a number of advantages. It's faster to run, requires less boilerplate code, has a nicer Python API, and doesn't require to run the Java RC server in parallel.
But it does also have a number of downsides. In particular it only supports Python 2.6 & 2.7, and some drivers (e.g. Chrome: http://code.google.com/p/selenium/wiki/ChromeDriver) are still lacking in functionality.
I think that's ok, though. The lacking drivers will catch up and Django is likely going to drop Python 2.5 support in the near future.
I've moved the base class (AdminSeleniumWebDriverTestCase
) for the admin tests to contrib/admin/tests.py
. You will also notice that the core of Django now doesn't contain any trace of Selenium and that the code for the RC flavour of Selenium present in previous patches has been removed. The approach here is similar with that of the one used for jQuery. Selenium and jQuery are both used by the admin, which in itself does represent a sort of "blessing", but this doesn't promote either of them as the one true way of handling respectively functional tests and dynamic interfaces in Django. This is partially to remain agnostic in regards to other frameworks and libraries, partially to avoid increasing the maintenance burden in Django core, and partially to make room for third party apps to develop more features around Selenium support. Again, 90% of the work is done by the built-in, generic, LiveServerTestCase
. Enabling Selenium in your test suite then becomes as simple as:
from django.test import LiveServerTestCase from selenium.webdriver.firefox.webdriver import WebDriver class MySeleniumTests(LiveServerTestCase): def setUp(self): self.selenium = WebDriver() super(MySeleniumTests, self).setUp() def tearDown(self): super(MySeleniumTests, self).tearDown() self.selenium.quit() def test_blah(self): ...
Any further feedback would be welcome. Thanks!
comment:75 by , 13 years ago
I don't know how big of a shift it would be but I've used Splinter for testing in order to get around Selenium deficiencies and it works on Python 2.5.
follow-up: 83 comment:76 by , 13 years ago
Interesting patch indeed, I have a few issue I'd like to see fixed first though:
- In
AdminSeleniumWebDriverTestCase
(shorter name?) thesetUp
method re-imports and instantiates the webdriver for every test method. Using the setUpClass class method would be preferred (unless I'm missing some need for resetting it all the time).
- The new
LIVE_TEST_SERVER_HOST
andLIVE_TEST_SERVER_PORT
settings are much too specific for Selenium (including the fact that "http" is hardcoded inLiveServerTestCase
). I suggest introducing justaLIVE_TEST_SERVER
setting instead and ask the users to give it a full URL, e.g. 'http://localhost/8081/'). Even though we have separate settings for email servers (EMAIL_HOST
andEMAIL_PORT
) the live test server has a lax API and shouldn't be hardwired to require protocol, host and port.
- The docs don't mention how to run the Selenium based tests except installing the selenium Python library. I suggest to add a small section about how to get Selenium to run in the first place (basically only where to download and how to run, like mentioned on http://pypi.python.org/pypi/selenium).
Other than that this looks good to me.
comment:77 by , 13 years ago
It looks like there's a race condition involved in reloading the database inside the LiveServerThread
.
It's sporadic, but if you run an empty LiveServerTestCase
test a few times you should see it manifest.
The process will fail to shutdown after the test completes.
class LiveServerTest(LiveServerTestCase): def test_live_server(self): pass
It looks like this was previously being masked because the Selenium tests were preventing the race from occuring.
I've narrowed things down to the Database creation in LiveServerThread.run
.
If that's commented out, or if it's moved into the __init__
(ie out of the new thread), the process shuts down fine.
I was thinking that it looked like the connection just needed to be explicitly closed and/or rolled back at the end of run()
,
but that didn't solve things - just made the race much less likely to occur, but still present occasionally.
I'm not exactly sure what's needed to fix it, so would appreciate some other eyes. :)
I'll try to find the time in the next couple of days to write up a few tests for LiveServerTestCase, that
use urllib2 or httplib to check the basics:
- Serving views.
- Serving 404s.
- Serving static media.
- Serving uploaded media, if MEDIA_URL is set. (I'm not sure this is done right now? It should be, right?)
- Ensure that fixtures are properly loaded in the live server thread.
- Ensure that model instances are properly created in the live server thread.
- Ensure that there's proper database seperation between test cases.
by , 13 years ago
Attachment: | 2879.selenium-support-extra-tests.diff added |
---|
comment:78 by , 13 years ago
Made a start on some extra tests...
./runtests.py --settings=test_sqlite live_server_tests
- As mentioned media files not served when
MEDIA_URL
andMEDIA_ROOT
set. (test_media_files
fails). Not 100% sure what the behavior ought to be here. - When running with in-memory database, database changes made in live server thread are not visible in test thread. (
test_database_writes
fails)... ...I'd assume(?) that'd pass on a proper database (not had time to test.) - When running with sqlite database using TEST_NAME in database settings, everything always hangs after "would you like to try deleting the test database" prompt.
- Multiple databases not yet supported? (not written a test for this yet.) Looks easy to fix.
- Process termination problem mentioned before can occur (Eg run
./runtests.py --settings=test_sqlite live_server_tests.TestViews.test_view
)
follow-up: 80 comment:79 by , 13 years ago
Thanks, Tom, those tests are very useful!
(By the way, there seems to be a problem with your latest patch as it didn't apply cleanly with 'git apply'. I had to use the 'patch -p1' command and then manually create the live_server_tests/__init__.py
file)
The process termination issue is a weird one. It seems to only occur with Python 2.7, as it works fine with Python 2.5&6. Also, adding sleep(0.001)
at the end of LiveServerTestCase.tearDown()
seems to fix it — although that wouldn't be acceptable as a solution :)
Threads cannot easily share memory in Python. Pysqlite addresses that problem by setting the check_same_thread
parameter to False
: sqlite.connect(":memory:", check_same_thread = False)
So, instead of recreating a new database in the separate thread, we should make sqlite share the same in-memory database by setting DATABASES['default']['OPTIONS']['check_same_thread']
to False in LiveServerTestCase.setUp()
. I haven't tested that yet. You can google 'check_same_thread' for more info — some people claim that it works and others that it doesn't. If no solution can be found then it looks like in-memory sqlite databases might not be supported by this approach.
As for the media, it's unclear to me from looking at your patch what you're actually testing. Where are the "example_media_file.txt" and "example_file.txt" files supposed to be physically located and served from?
comment:80 by , 13 years ago
Replying to julien:
So, instead of recreating a new database in the separate thread, we should make sqlite share the same in-memory database by setting
DATABASES['default']['OPTIONS']['check_same_thread']
to False inLiveServerTestCase.setUp()
. I haven't tested that yet. You can google 'check_same_thread' for more info — some people claim that it works and others that it doesn't. If no solution can be found then it looks like in-memory sqlite databases might not be supported by this approach.
This totally depends on whether test wants to share data / how transactions should behave. In django-sane-testing, we opted to skip the live server tests when using in-memory SQLite database, because one cannot correctly predict the test intentions.
by , 13 years ago
Attachment: | 2879.selenium-support-extra-tests.2.diff added |
---|
As before, but include example static and media files, and slightly more explicit intended test behavior for media/static files
comment:81 by , 13 years ago
Not sure, but you might need to add the new directories prior to applying the patch:
mkdir -p tests/regressiontests/live_server_tests/{media,static}
I'd missed out the example media and static file before - should be more clear now.
So there's a test file in each of the static
and media
directories, and we'd expect them to serve from /static/
and /media/
.
We're already making the implicit assumption that we want to serve static files, with the equivalent of:
urlpatterns += staticfiles_urlpatterns()
If MEDIA_ROOT
is set, and MEDIA_URL
is set to a relative URL, then we ought to also do the equivalent of:
urlpatterns += static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT)
(As it happens, I actually think Django's "use settings.DEBUG
to determine if we should serve static/media files" is a slightly flawed in the first place, seeing as it's forced to False
in testing.)
I might take a look at seeing if I can adapt the implementation to be multi-process rather than multi-thread, and support on-disk sqlite databases.
by , 13 years ago
Attachment: | 2879.selenium-support.7.diff added |
---|
comment:82 by , 13 years ago
I've spent some time trying to make it work for in-memory sqlite databases but unfortunately it looks like this may not be achievable. So, as suggested by Almad, I've made it to skip the tests in that case.
Also, you'll note that LiveServerTestCase
now inherits from TransactionTestCase
instead of TestCase
. The problem with TestCase
is that changes are not committed and therefore the live server's database connection can't have access to them since it uses a different cursor.
Finally, I've made the live server serve media files by introducing a new MediaFilesHandler
class based on StaticFilesHandler
. Maybe there's a better approach so, as always, any feedback is welcome! :)
PS: Tom, I've had some issues applying your previous patch. In particular some new files seemed to be missing. Are you creating the diff from the working directory? If so, I usually achieve this by staging all the files I want to diff and then by running: git diff --staged > mypatch.diff
. Although it feels there has to be a better way!
by , 13 years ago
Attachment: | 2879.selenium-support.8.diff added |
---|
follow-up: 84 comment:83 by , 13 years ago
Replying to jezdez:
- In
AdminSeleniumWebDriverTestCase
(shorter name?) thesetUp
method re-imports and instantiates the webdriver for every test method. Using the setUpClass class method would be preferred (unless I'm missing some need for resetting it all the time).
Agreed. I've made that change in the latest patch.
- The new
LIVE_TEST_SERVER_HOST
andLIVE_TEST_SERVER_PORT
settings are much too specific for Selenium (including the fact that "http" is hardcoded inLiveServerTestCase
). I suggest introducing justaLIVE_TEST_SERVER
setting instead and ask the users to give it a full URL, e.g. 'http://localhost/8081/'). Even though we have separate settings for email servers (EMAIL_HOST
andEMAIL_PORT
) the live test server has a lax API and shouldn't be hardwired to require protocol, host and port.
Internally the WSGIServer
opens a socket and binds it to a server address that has to be a pair (host, port), so we might have to keep that API too. What do you think?
- The docs don't mention how to run the Selenium based tests except installing the selenium Python library. I suggest to add a small section about how to get Selenium to run in the first place (basically only where to download and how to run, like mentioned on http://pypi.python.org/pypi/selenium).
As discussed on IRC, it is really as simple as installing the selenium package. The documentation could perhaps be improved, but it already contains all you need to get started.
Finally, in the latest patch I've moved the live server tests to the already existing 'servers' app, and made a few small tweaks to the admin selenium tests.
comment:84 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Replying to julien:
Replying to jezdez:
- The new
LIVE_TEST_SERVER_HOST
andLIVE_TEST_SERVER_PORT
settings are much too specific for Selenium (including the fact that "http" is hardcoded inLiveServerTestCase
). I suggest introducing justaLIVE_TEST_SERVER
setting instead and ask the users to give it a full URL, e.g. 'http://localhost/8081/'). Even though we have separate settings for email servers (EMAIL_HOST
andEMAIL_PORT
) the live test server has a lax API and shouldn't be hardwired to require protocol, host and port.Internally the
WSGIServer
opens a socket and binds it to a server address that has to be a pair (host, port), so we might have to keep that API too. What do you think?
Ah, yeah, I misinterpreted the use. Looks good to me now.
- The docs don't mention how to run the Selenium based tests except installing the selenium Python library. I suggest to add a small section about how to get Selenium to run in the first place (basically only where to download and how to run, like mentioned on http://pypi.python.org/pypi/selenium).
As discussed on IRC, it is really as simple as installing the selenium package. The documentation could perhaps be improved, but it already contains all you need to get started.
Yeah, I think this is fine for committing.
Finally, in the latest patch I've moved the live server tests to the already existing 'servers' app, and made a few small tweaks to the admin selenium tests.
Woo!
by , 13 years ago
Attachment: | 2879.selenium-support.9.diff added |
---|
comment:86 by , 13 years ago
Cc: | added |
---|
Spotted one comment which is a bit misleading.
The comment containing the snippet "the threads do not share the same connection's cursor (unless if using in-memory sqlite)." isn't exactly correct: This is not about sharing cursors, but that in general the setup must be:
TestCase
: sets up database state, commits. Makes request (by selenium or whatever have you).TestServer
: serves the request normally, this means usually a transaction is made and committed.TestCase
: checks database state (yet another transaction).
So, that comment might need updating. Also, it would be good to document the above. Maybe all that is needed is: "remember to commit changes to db-state before making requests". If there is no commit, the test-server thread's connection will not see those changes, unless using inmemory-sqlite.
comment:87 by , 13 years ago
Cc: | added |
---|
comment:88 by , 13 years ago
Some very minor docs feedback:
1844:
:class:~django.test.TransactionTestCase
with one extra thing: it launches a
:class:~django.test.TransactionTestCase
with one extra feature: it launches a
1846:
This allows to use other automated test clients than the
This allows the use of automated test clients other than the
Looks like a great patch!
comment:89 by , 13 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
ptone: thanks for your docs feedback!
akaariai: Are you suggesting that only the comment be changed and/or that LiverServerTestCase
should inherit from TestCase
instead of TransactionTestCase
?
Also, it's been suggested on IRC that to avoid adding 2 new settings the live server's host and port could be passed as options to the manage.py test
command. That sounds like a good idea, but at this stage I'm wondering how this could work if one is using a different test runner than Django's.
comment:90 by , 13 years ago
What I am trying to say is that a) the comment refers to connection's cursor sharing (when in my opinion it should say something about shared transactions) and b) the test methods must be written as:
def test_something(): do_setup() commit # MUST commit if you are altering the database in some way # and wish it to be visible to the live-server. If no changes, # commit is of course optional. selenium.do_request() # the test-server can now run in its own transaction, # which can see things done in do_setup() inspect_results()
My main point is that if you do per-test-method setup which write to the DB, you must commit those changes before doing the request to the test-server. Otherwise, the setup will not be visible to the test-server when it handles the request.
Of course, if you are doing more than one request to the db in single test, and doing more setup between the request, you must commit the changes between each request. So before each request, commit changes (if anything to commit).
I do not see transaction commits forced in code, or mentioned in the documentation. I think this should at least be mentioned in the documentation. When writing this, I am beginning to think that the correct solution is to enforce all open transactions are non-dirty before request are made. But enforcing that seems problematic...
follow-up: 92 comment:91 by , 13 years ago
Re the above transaction handling: it seems that when running the test_methods, you are not running in a managed transaction state. So, all saves will be committed immediately. Thus, the above might not be such a big problem after all.
While testing this patch, I got this error:
Exception in thread Thread-1: Traceback (most recent call last): File "/usr/lib/python2.6/threading.py", line 532, in __bootstrap_inner self.run() File "/home/akaj/Django/django_test/django/test/testcases.py", line 861, in run handler = StaticFilesHandler(MediaFilesHandler(WSGIHandler())) File "/home/akaj/Django/django_test/django/contrib/staticfiles/handlers.py", line 21, in __init__ self.base_url = urlparse(self.get_base_url()) File "/home/akaj/Django/django_test/django/contrib/staticfiles/handlers.py", line 28, in get_base_url utils.check_settings() File "/home/akaj/Django/django_test/django/contrib/staticfiles/utils.py", line 49, in check_settings "You're using the staticfiles app " ImproperlyConfigured: You're using the staticfiles app without having set the required STATIC_URL setting.
The test_runner just hang. ctrl-C does nothing. So 1) it seems the StaticFilesHandler
needs STATIC_URL. 2) If you get an error in the setup of the live-server, the thread will not go away. I took a quick glance, and can't see why it doesn't go away. And 3) the live-server thread should be daemonic to minimize the potential for process-hangs.
I also got this printout for a simple test (one get request, test one element present).
python manage.py test --settings=settings obj_creation_speed Creating test database for alias 'default'... Traceback (most recent call last): File "/usr/lib/python2.6/wsgiref/handlers.py", line 93, in run self.result = application(self.environ, self.start_response) File "/home/akaj/Django/django_test/django/contrib/staticfiles/handlers.py", line 67, in __call__ return self.application(environ, start_response) File "/home/akaj/Django/django_test/django/contrib/staticfiles/handlers.py", line 68, in __call__ return super(StaticFilesHandler, self).__call__(environ, start_response) File "/home/akaj/Django/django_test/django/core/handlers/wsgi.py", line 242, in __call__ response = self.get_response(request) File "/home/akaj/Django/django_test/django/contrib/staticfiles/handlers.py", line 63, in get_response return super(StaticFilesHandler, self).get_response(request) File "/home/akaj/Django/django_test/django/core/handlers/base.py", line 153, in get_response response = self.handle_uncaught_exception(request, resolver, sys.exc_info()) File "/home/akaj/Django/django_test/django/core/handlers/base.py", line 228, in handle_uncaught_exception return callback(request, **param_dict) File "/home/akaj/Django/django_test/django/utils/decorators.py", line 91, in _wrapped_view response = view_func(request, *args, **kwargs) File "/home/akaj/Django/django_test/django/views/defaults.py", line 32, in server_error t = loader.get_template(template_name) # You need to create a 500.html template. File "/home/akaj/Django/django_test/django/template/loader.py", line 145, in get_template template, origin = find_template(template_name) File "/home/akaj/Django/django_test/django/template/loader.py", line 138, in find_template raise TemplateDoesNotExist(name) TemplateDoesNotExist: 500.html Traceback (most recent call last): File "/usr/lib/python2.6/wsgiref/handlers.py", line 93, in run self.result = application(self.environ, self.start_response) File "/home/akaj/Django/django_test/django/contrib/staticfiles/handlers.py", line 67, in __call__ return self.application(environ, start_response) File "/home/akaj/Django/django_test/django/contrib/staticfiles/handlers.py", line 68, in __call__ return super(StaticFilesHandler, self).__call__(environ, start_response) File "/home/akaj/Django/django_test/django/core/handlers/wsgi.py", line 242, in __call__ response = self.get_response(request) File "/home/akaj/Django/django_test/django/contrib/staticfiles/handlers.py", line 63, in get_response return super(StaticFilesHandler, self).get_response(request) File "/home/akaj/Django/django_test/django/core/handlers/base.py", line 153, in get_response response = self.handle_uncaught_exception(request, resolver, sys.exc_info()) File "/home/akaj/Django/django_test/django/core/handlers/base.py", line 228, in handle_uncaught_exception return callback(request, **param_dict) File "/home/akaj/Django/django_test/django/utils/decorators.py", line 91, in _wrapped_view response = view_func(request, *args, **kwargs) File "/home/akaj/Django/django_test/django/views/defaults.py", line 32, in server_error t = loader.get_template(template_name) # You need to create a 500.html template. File "/home/akaj/Django/django_test/django/template/loader.py", line 145, in get_template template, origin = find_template(template_name) File "/home/akaj/Django/django_test/django/template/loader.py", line 138, in find_template raise TemplateDoesNotExist(name) TemplateDoesNotExist: 500.html . ---------------------------------------------------------------------- Ran 1 test in 9.317s OK Destroying test database for alias 'default'...
So, I am getting a couple of 500.html not exists for a test which is (as far as I understand) doing just a single get.
follow-up: 93 comment:92 by , 13 years ago
Replying to akaariai:
Re the above transaction handling: it seems that when running the test_methods, you are not running in a managed transaction state. So, all saves will be committed immediately. Thus, the above might not be such a big problem after all.
Yes, this was the motivation for inheriting from TransactionTestCase
instead of TestCase
. It means that the tests would be slightly slower but it makes writing tests for Selenium (and other frameworks) much more friendly. Suggestions for changing the code or docs are welcome though.
While testing this patch, I got this error:
Exception in thread Thread-1: Traceback (most recent call last): File "/usr/lib/python2.6/threading.py", line 532, in __bootstrap_inner self.run() <SNIP> File "/home/akaj/Django/django_test/django/contrib/staticfiles/utils.py", line 49, in check_settings "You're using the staticfiles app " ImproperlyConfigured: You're using the staticfiles app without having set the required STATIC_URL setting.The test_runner just hang. ctrl-C does nothing. So 1) it seems the
StaticFilesHandler
needs STATIC_URL. 2) If you get an error in the setup of the live-server, the thread will not go away. I took a quick glance, and can't see why it doesn't go away. And 3) the live-server thread should be daemonic to minimize the potential for process-hangs.
1) The live server makes use of STATIC_URL
and yes it needs to be provided.
2) We definitely need to change that behaviour if it's repeatable.
I also got this printout for a simple test (one get request, test one element present).
python manage.py test --settings=settings obj_creation_speed Creating test database for alias 'default'... Traceback (most recent call last): <SNIP> File "/home/akaj/Django/django_test/django/template/loader.py", line 138, in find_template raise TemplateDoesNotExist(name) TemplateDoesNotExist: 500.html . ---------------------------------------------------------------------- Ran 1 test in 9.317s OK Destroying test database for alias 'default'...So, I am getting a couple of 500.html not exists for a test which is (as far as I understand) doing just a single get.
This probably is due to the fact that the browser is trying to get favicon.ico from the root of the live test server's url. Also your project doesn't define a 500.html. If you add one, do you get a more specific description of the root problem?
comment:93 by , 13 years ago
Replying to julien:
Replying to akaariai:
Re the above transaction handling: it seems that when running the test_methods, you are not running in a managed transaction state. So, all saves will be committed immediately. Thus, the above might not be such a big problem after all.
Yes, this was the motivation for inheriting from
TransactionTestCase
instead ofTestCase
. It means that the tests would be slightly slower but it makes writing tests for Selenium (and other frameworks) much more friendly. Suggestions for changing the code or docs are welcome though.
I don't see any big problem anymore about the transaction handling. I thought the test_method SQL would run in a transaction (and it actually does), but as it is not a managed transaction, all data modifying operations will commit immediately, at least as long as you use the ORM. I don't have any suggestions for improvement.
1) The live server makes use of
STATIC_URL
and yes it needs to be provided.
2) We definitely need to change that behaviour if it's repeatable.
It is probably a good idea to make the live-server thread daemonic in any case. That will allow the test-runner process exit even if the live-server is left alive for some reason. It doesn't have to be error in Django, the user might have an endless loop by accident in one of his views or something like that.
If I have time, I will check tomorrow if I find anything about why the live-server thread gets stuck. It should be easy to reproduce the stuck thread problem. Setup a new project, make sure you don't have STATIC_URL
defined, run some tests using LiveServerTestCase
and see what happens.
This probably is due to the fact that the browser is trying to get favicon.ico from the root of the live test server's url. Also your project doesn't define a 500.html. If you add one, do you get a more specific description of the root problem?
Ah, this makes sense. Again, will check tomorrow about this, although it seems there isn't much to check about.
comment:94 by , 13 years ago
Ok, I did some investigation, and now STATIC_URL and quitting the tests when the server hits forever-loop should work.
Attached is an incremental patch for the .10. patch. The stuff in my patch isn't polished at all, but it should show the problematic places, and at least some way to fix them.
One thing about documentation: I wonder if it is a good advice to do selenium setup / quit in tearDown and setUp. This makes each test_method take about 7 seconds on my system (for each test method, start and stop Firefox). Would it be better to advice the use of tearDownClass and setUpClass? This way Firefox is started only once per test class. On the other hand, this has a bit bigger chance of hitting cross-test dependencies. Maybe at least mention in the documentation that this is also possible? From my point of view, this documentation polishing can be done after initial commit.
by , 13 years ago
Attachment: | 2879.selenium-support.11.diff added |
---|
comment:95 by , 13 years ago
akaariai: Your suggestion for preventing dead locks when shutting down the live server makes sense. I've made the change in the latest patch, although I've borrowed some code from Python 2.7 to do the implementation (see the _ImprovedEvent
class). I also agree about documenting the use of setUpClass/tearDownClass and including that change in the latest patch.
However, I'm not sure how your suggestion addresses the STATIC_URL
issue. Could you elaborate on that? I haven't had the time to investigate it fully myself yet. It seems at the very least there should be some documentation about it.
From what I recall in the early days that I worked on this patch, I now remember having hit some issues if the tested site didn't include a favicon.ico, 404.html or 500.html files. Either the code should handle those cases or that should be documented.
comment:96 by , 13 years ago
The live-server still requires STATIC_URL, it just doesn't get stuck if it is missing. So, it is fixed only in that sense.
It would be nice if the stacktraces for missing 500.html would not get printed. But it isn't that important...
by , 13 years ago
Attachment: | 2879.selenium-support.12.diff added |
---|
comment:97 by , 13 years ago
Patch needs improvement: | unset |
---|
The latest (and hopefully last) patch doesn't use settings anymore. Instead it uses a single environment variable holding the default address 'localhost:8081'. To override that default address, one can pass the --liveserver
option to the 'manage.py test
' command.
By the way, I've figured out why a missing STATIC_URL used to cause the test to freeze. It was because the generated exception didn't bubble up to the main thread, which in turn could not release the live server's socket. It is still worth leaving the safety shutdown mechanism for the case where the user has an infinite loop in their views though. This is now all fixed and tested in the new patch.
comment:98 by , 13 years ago
Still a couple of minor things.
If I am not mistaken this part of admin_login will not work in i18n setting: It is not "Log in" in Finnish :)
self.selenium.find_element_by_xpath('//input[@value="Log in"]').click()
The same language-dependency is also visible in the admin_inlines tests. These can probably be fixed later on.
A small nitpick: some method docstrings are written as:
"""Comment begins here ... """
whereas I believe the recommended style is:
""" Comment begins on separate line... """
Last nitpick is the handling of exceptions in LiveServerThread
.run(). You fixed the exception from missing STATIC_URL, but I think it would be good to have
except Exception, e: self.e = e self.is_ready.set() raise
in the end of the run method, below the except WSGIServerException
. It is unlikely the general "except Exception:" is ever hit, but if it is for some reason (out of memory or something...) this will prevent lockup.
As said, just minor nitpicks. I haven't fully tested the latest patch, but I think it is possible to commit this as is, and then fix the issues once this is committed.
comment:99 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:101 by , 13 years ago
My sincere apologies to Florian Apolloner and Jonas Obrist, who I've failed to mention in the commit message and who also provided excellent feedback!
comment:102 by , 13 years ago
Cc: | added |
---|
path to resole this enhancement