Opened 8 years ago

Closed 3 years ago

Last modified 3 years ago

#2879 closed New feature (fixed)

Add live test server support to test framework

Reported by: Mikeal Rogers <mikeal@…> Owned by: devin
Component: Testing framework Version:
Severity: Normal Keywords:
Cc: russellm, allandouglas@…, dnaquin@…, jbeigel, andrew@…, mtredinnick, wiswaud, martin@…, simon@…, alex@…, zodizz@…, robert.ramirez@…, gabor@…, clouserw@…, Goldan, phartig@…, roman@…, dwight@…, lrekucki@…, kmike84@…, ShawnMilo, bradley.ayers@…, tom@…, frankoid, 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)

http_test_case.diff (4.5 KB) - added by Mikeal Rogers <mikeal@…> 8 years ago.
path to resole this enhancement
http_test_case.2.diff (4.5 KB) - added by Mikeal Rogers <mikeal@…> 8 years ago.
fixed comment typo and improper print statement
core-management.patch (1.9 KB) - added by Dirk Datzert <dummy@…> 8 years ago.
runtestserver command for manage.py
testing.patch (1.4 KB) - added by Dirk Datzert <dummy@…> 8 years ago.
testing contrib to flush database during tests
test-server-support.patch (1.9 KB) - added by Almad <bugs@…> 8 years ago.
All patches in one, with reverted utils.py
run_test_server_simple.py.diff (1.2 KB) - added by Denis Golomazov <denis.golomazov@…> 7 years ago.
run_test_server_utils.py.diff (707 bytes) - added by Denis Golomazov <denis.golomazov@…> 7 years ago.
The second part of the patch
django_live_server.diff (8.0 KB) - added by devin 7 years ago.
fixes way server handles error on startup
django_live_server_r7936.diff (8.7 KB) - added by devin 7 years ago.
refactor and cleanup
django_live_server_r8458.diff (8.9 KB) - added by robertrv 7 years ago.
Updated devin's patch with last changes in testing environment
django_live_server_1.0.2.diff (8.9 KB) - added by andrewbadr 6 years ago.
untested patch for Django 1.0.2
2879.selenium-support.diff (21.5 KB) - added by julien 3 years ago.
2879.selenium-support.2.diff (24.0 KB) - added by julien 3 years ago.
2879.selenium-support.3.diff (24.6 KB) - added by julien 3 years ago.
Works nicer with Python 2.7
2879.selenium-support.4.diff (28.6 KB) - added by julien 3 years ago.
2879.selenium-support.5.diff (27.3 KB) - added by tomchristie 3 years ago.
doh.diff (3.0 KB) - added by tomchristie 3 years ago.
django.contrib.selenium for previous diff
2879.selenium-support.6.diff (26.9 KB) - added by julien 3 years ago.
Now using WebDriver
2879.selenium-support-extra-tests.diff (30.6 KB) - added by tomchristie 3 years ago.
2879.selenium-support-extra-tests.2.diff (31.0 KB) - added by tomchristie 3 years ago.
As before, but include example static and media files, and slightly more explicit intended test behavior for media/static files
2879.selenium-support.7.diff (32.1 KB) - added by julien 3 years ago.
2879.selenium-support.8.diff (35.8 KB) - added by julien 3 years ago.
2879.selenium-support.9.diff (42.8 KB) - added by julien 3 years ago.
2879.selenium-support.10.diff (37.1 KB) - added by julien 3 years ago.
Hopefully the last patch :)
2879-error-fixes.diff (1.5 KB) - added by akaariai 3 years ago.
Incremental to .10.diff
2879.selenium-support.11.diff (38.2 KB) - added by julien 3 years ago.
2879.selenium-support.12.diff (45.9 KB) - added by julien 3 years ago.

Download all attachments as: .zip

Change History (129)

Changed 8 years ago by Mikeal Rogers <mikeal@…>

path to resole this enhancement

comment:1 Changed 8 years ago by Mikeal Rogers <mikeal@…>

  • Owner changed from Mikeal Rogers to anonymous
  • Status changed from new to assigned
  • Summary changed from Add live test server support to test framework to [patch] Add live test server support to test framework

Adding patch to subject.

comment:2 Changed 8 years ago by Mikeal Rogers <mikeal@…>

  • Owner changed from anonymous to Mikeal Rogers
  • Status changed from assigned to new

Whoops.

Changed 8 years ago by Mikeal Rogers <mikeal@…>

fixed comment typo and improper print statement

comment:3 Changed 8 years ago by Mikeal Rogers <mikeal@…>

  • Owner Mikeal Rogers deleted

Un-assigning from myself since this needs to be reviewed by a commiter.

comment:4 Changed 8 years ago by russellm

  • Owner set to russellm

comment:5 Changed 8 years ago by mir@…

How does this relate to the unit test system that has evolved since then?

comment:6 Changed 8 years ago by mikeal

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 Changed 8 years ago by mir@…

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 Changed 8 years ago by mir@…

  • Cc russellm 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 Changed 8 years ago by mikeal

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.

Changed 8 years ago by Dirk Datzert <dummy@…>

runtestserver command for manage.py

comment:10 Changed 8 years ago by Dirk Datzert <dummy@…>

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'.

Changed 8 years ago by Dirk Datzert <dummy@…>

testing contrib to flush database during tests

comment:11 Changed 8 years ago by Dirk Datzert <dummy@…>

Together with the core-management.patch it would be possible to flush a running live test server between tests.

comment:12 Changed 8 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Design decision needed

comment:13 Changed 8 years ago by Almad <bugs@…>

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.

Changed 8 years ago by Almad <bugs@…>

All patches in one, with reverted utils.py

comment:14 Changed 8 years ago by anonymous

  • Cc allandouglas@… added

comment:15 Changed 7 years ago by russellm

  • Triage Stage changed from Design decision needed to Accepted

comment:16 Changed 7 years ago by ubernostrum

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

I believe this was fixed in [5912] with the addition of manage.py testserver.

comment:17 Changed 7 years ago by russellm

  • Resolution fixed deleted
  • Status changed from closed to 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 Changed 7 years ago by bugs@…

Is there a way to see this one in trunk?

Anything needs to be done, or any way how to speed it up?

comment:19 follow-up: Changed 7 years ago by Mikeal Rogers <mikeal@…>

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 Changed 7 years ago by Mikeal Rogers <mikeal@…>

Sorry, i was trying to apply the wrong patch.

Haven't tried the newest set.

comment:21 in reply to: ↑ 19 Changed 7 years ago by russellm

  • 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:

  1. 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.
  2. 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.
  3. 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.
  4. (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.

comment:22 follow-up: Changed 7 years ago by Mikeal Rogers <mikeal@…>

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 in reply to: ↑ 22 Changed 7 years ago by Almad <bugs@…>

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

Changed 7 years ago by Denis Golomazov <denis.golomazov@…>

comment:24 Changed 7 years ago by Denis Golomazov <denis.golomazov@…>

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.

Changed 7 years ago by Denis Golomazov <denis.golomazov@…>

The second part of the patch

comment:25 Changed 7 years ago by Denis Golomazov <denis.golomazov@…>

Sorry, but I can't figure out why diff for the first file isn't displayed.

comment:26 Changed 7 years ago by devin

  • Owner changed from nobody to devin
  • Status changed from reopened to 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 Changed 7 years ago by devin

  • 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 Changed 7 years ago by devin

  • Cc dnaquin@… added

comment:29 Changed 7 years ago by ubernostrum

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 Changed 7 years ago by devin

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 Changed 7 years ago by jbeigel

  • Cc jbeigel added

comment:32 Changed 7 years ago by anonymous

  • Cc andrew@… added

comment:33 Changed 7 years ago by devin

  • 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.

Changed 7 years ago by devin

fixes way server handles error on startup

comment:34 Changed 7 years ago by devin

  • Patch needs improvement unset

Changed 7 years ago by devin

refactor and cleanup

comment:35 Changed 7 years ago by mtredinnick

  • Cc mtredinnick added

comment:36 Changed 7 years ago by wiswaud

  • Cc wiswaud martin@… simon@… added

comment:37 Changed 7 years ago by alex@…

  • Cc alex@… added

comment:38 Changed 7 years ago by devin

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 Changed 7 years ago by devin

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.

comment:40 follow-up: Changed 7 years ago by anonymous

  • Cc zodizz@… added

comment:41 Changed 7 years ago by anonymous

  • Cc robert.ramirez@… added

Changed 7 years ago by robertrv

Updated devin's patch with last changes in testing environment

comment:42 follow-up: Changed 6 years ago by mikeal

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:44 Changed 6 years ago by gabor

  • Cc gabor@… added

comment:45 in reply to: ↑ 42 Changed 6 years ago by Almad

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.

Changed 6 years ago by andrewbadr

untested patch for Django 1.0.2

comment:46 Changed 5 years ago by clouserw

  • Cc clouserw@… added

comment:47 Changed 5 years ago by leonardopessoa

any progress or changes to work on 1.1.1? I don't seem to find it anywhere

comment:48 Changed 5 years ago by Goldan

  • Cc Goldan added

comment:49 Changed 5 years ago by adamnelson

  • Patch needs improvement set

Last two patches appear to be corrupted.

comment:50 Changed 5 years ago by ch0wn

  • Cc phartig@… added

comment:51 Changed 4 years ago by t0ster

  • Cc roman@… added

comment:52 Changed 4 years ago by dwight@…

  • Cc dwight@… added

comment:53 Changed 4 years ago by lrekucki

  • Severity changed from normal to Normal
  • Type changed from enhancement to New feature

comment:54 Changed 4 years ago by lrekucki

  • Cc lrekucki@… added
  • Summary changed from [patch] Add live test server support to test framework to 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 Changed 4 years ago by kmike

  • Cc kmike84@… added
  • Easy pickings unset

comment:56 Changed 4 years ago by ShawnMilo

  • Cc ShawnMilo added
  • UI/UX unset

comment:57 Changed 3 years ago by julien

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!

Changed 3 years ago by julien

comment:58 Changed 3 years ago by julien

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 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

  • Cc bradley.ayers@… added

Changed 3 years ago by julien

comment:60 Changed 3 years ago by julien

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!

Changed 3 years ago by julien

Works nicer with Python 2.7

Changed 3 years ago by julien

comment:61 Changed 3 years ago by julien

The patch now comes with full documentation and is candidate for RFC. Any feedback welcome. Thanks!

comment:62 Changed 3 years ago by idangazit

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 Changed 3 years ago by tomchristie

  • Cc tom@… added

comment:64 Changed 3 years ago by frankoid

  • Cc frankoid added

comment:65 Changed 3 years ago by hjwp2@…

+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 Changed 3 years ago by hjwp2@…

  • Cc hjwp2@… added

comment:67 Changed 3 years ago by julien

@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 Changed 3 years ago by tomchristie

Some thoughts:

  1. 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 to SeleniumRCTestCase. Supporting WebDriver could then be a separate ticket.
  2. 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.
  3. Would it be worth considering moving SeleniumTestCase (or SeleniumRCTestCase and SeleniumWebDriverTestCase) into a new package django.contrib.selenium? (Obviously LiveServerTest would stay in django.test)

comment:69 Changed 3 years ago by tomchristie

Attached patch does the following:

  1. Rename SeleniumTestCase to SeleniumRCTestCase, settings to SELENIUM_RC_HOST, SELENIUM_RC_PORT, SELENIUM_RC_BROWSER, and updates docs accordingly.
  2. Move SeleniumTestCase into django.contrib.selenium.
  3. Check for Selenium connection during the first test run, rather than at time of import.
  4. Move "Wait for the Django server to be ready" code out of SeleniumTestCase, into LiveServerTestCase.

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.

Changed 3 years ago by tomchristie

comment:70 Changed 3 years ago by julien

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 Changed 3 years ago by julien

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.

Changed 3 years ago by tomchristie

django.contrib.selenium for previous diff

comment:72 Changed 3 years ago by tomchristie

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 Changed 3 years ago by voidspace

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.

Changed 3 years ago by julien

Now using WebDriver

comment:74 Changed 3 years ago by julien

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 Changed 3 years ago by adamnelson

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.

comment:76 follow-up: Changed 3 years ago by jezdez

Interesting patch indeed, I have a few issue I'd like to see fixed first though:

  • In AdminSeleniumWebDriverTestCase (shorter name?) the setUp 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 and LIVE_TEST_SERVER_PORT settings are much too specific for Selenium (including the fact that "http" is hardcoded in LiveServerTestCase). I suggest introducing justa LIVE_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 and EMAIL_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 Changed 3 years ago by tomchristie

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.

Changed 3 years ago by tomchristie

comment:78 Changed 3 years ago by tomchristie

Made a start on some extra tests...

./runtests.py --settings=test_sqlite live_server_tests
  1. As mentioned media files not served when MEDIA_URL and MEDIA_ROOT set. (test_media_files fails). Not 100% sure what the behavior ought to be here.
  2. 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.)
  3. 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.
  4. Multiple databases not yet supported? (not written a test for this yet.) Looks easy to fix.
  5. Process termination problem mentioned before can occur (Eg run ./runtests.py --settings=test_sqlite live_server_tests.TestViews.test_view)

comment:79 follow-up: Changed 3 years ago by julien

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 in reply to: ↑ 79 Changed 3 years ago by Almad

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

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.

Changed 3 years ago by tomchristie

As before, but include example static and media files, and slightly more explicit intended test behavior for media/static files

comment:81 Changed 3 years ago by tomchristie

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.

Changed 3 years ago by julien

comment:82 Changed 3 years ago by julien

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!

Changed 3 years ago by julien

comment:83 in reply to: ↑ 76 ; follow-up: Changed 3 years ago by julien

Replying to jezdez:

  • In AdminSeleniumWebDriverTestCase (shorter name?) the setUp 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 and LIVE_TEST_SERVER_PORT settings are much too specific for Selenium (including the fact that "http" is hardcoded in LiveServerTestCase). I suggest introducing justa LIVE_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 and EMAIL_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 in reply to: ↑ 83 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

Replying to julien:

Replying to jezdez:

  • The new LIVE_TEST_SERVER_HOST and LIVE_TEST_SERVER_PORT settings are much too specific for Selenium (including the fact that "http" is hardcoded in LiveServerTestCase). I suggest introducing justa LIVE_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 and EMAIL_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!

Changed 3 years ago by julien

comment:85 Changed 3 years ago by julien

In [17205]:

(The changeset message doesn't reference this ticket)

Changed 3 years ago by julien

Hopefully the last patch :)

comment:86 Changed 3 years ago by akaariai

  • Cc anssi.kaariainen@… 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 Changed 3 years ago by unbracketed

  • Cc unbracketed added

comment:88 Changed 3 years ago by ptone

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 Changed 3 years ago by julien

  • Triage Stage changed from Ready for checkin to 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 Changed 3 years ago by akaariai

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

comment:91 follow-up: Changed 3 years ago by 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.

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.

comment:92 in reply to: ↑ 91 ; follow-up: Changed 3 years ago by 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 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 in reply to: ↑ 92 Changed 3 years ago by akaariai

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

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 Changed 3 years ago by akaariai

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.

Changed 3 years ago by akaariai

Incremental to .10.diff

Changed 3 years ago by julien

comment:95 Changed 3 years ago by julien

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 Changed 3 years ago by akaariai

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

Changed 3 years ago by julien

comment:97 Changed 3 years ago by julien

  • 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 Changed 3 years ago by akaariai

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 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:100 Changed 3 years ago by julien

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

In [17241]:

(The changeset message doesn't reference this ticket)

comment:101 Changed 3 years ago by julien

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 Changed 3 years ago by jian@…

  • Cc jian@… added
Note: See TracTickets for help on using tickets.
Back to Top