Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1298 (closed defect: fixed)

Opened 12 years ago

Last modified 12 years ago

[PATCH] improved quickstart controller test

Reported by: chrisz Owned by:
Priority: normal Milestone: 1.0.2
Component: unassigned Version: 1.0
Severity: normal Keywords: nosetests controller
Cc:

Description

The quickstart controller test shuts down TurboGears after each test, if quickstart has been run with identity management, in order to stop the visit manager thread (see #1217). Starting up TurboGears explicitely is actually not necessary since this is done automatically when requests are created using testutil. However, I think it is better to startup and shutdown TurboGears explicitely for each controller test, even if identity management has not been set up on quickstart, to provide the user with a clean and comprehensible standard test environment. If users later decide to activate identity management and the shutdown is not in the controller tests, they will get confusing errors from the unit test environment, and they will not know whether TurboGears is started in the tests or not. Also, I have changed the test to an ordinary unittest so it does not depend on nosetests any more (though it still runs with nosetests, of course).

Attachments

qs_controller_test.patch Download (2.6 KB) - added by chrisz 12 years ago.
Quickstart controller test patch (for trunk and 1.0 branch)

Change History

Changed 12 years ago by chrisz

Quickstart controller test patch (for trunk and 1.0 branch)

comment:1 Changed 12 years ago by chrisz

  • Type changed from enhancement to defect
  • Severity changed from minor to normal

This patch also ensures that the identity tables exist for every test. This is important because the tables are created in memory only for the tests, and are destroyed after a shutdown.

comment:2 Changed 12 years ago by jorge.vargas

I don't think starting/stoping TG for each test is a good idea, why if there is a bug after the second request or going from one page to the other?

is there any particular reason why you took out nose? nose is the std. testing engine for TG.

comment:3 Changed 12 years ago by chrisz

I just made it a standard unittest so that you can run it without nosetest as well, and because setup and teardown is simpler if you do a test-wise setup and teardown as it is done currently (at least the teardown). You can still run it with nosetests. The other TG unittests are not dependent from nosetest either as far as I can see.

The shutdown is necessary in order to stop the visit manager thread. It should happen in the teardown method so that you don't get a confusing message if a test fails. The startup is necessary because the tables are destroyed on shutdown since a sqlite memory database is used for the tests, so you need to recreate the tables.

This also cleanly separates the tests from each other. If you are worried about performance issues, you can use setup and teardown for the whole module instead (with nosetest only). However, if we use setup and teardown for every test function, then it is easier to use the standard unittest mechanism.

comment:4 Changed 12 years ago by chrisz

Also, the idea of unittesting is to check the single controller method only, not the behavior when going from one page to the other. If you want to do this, you should make a test that does exactly that, i.e. create a flow of requests inside one test method and check whether that works. This would be nice to have for the identity functionality, i.e. you could check whether you can access a protected page only after logging in correctly. But this needs to be inside a single test then so you can be sure you are not logged in already due to earlier tests, and actually, this would be already more like an acceptance test than a unittest.

comment:5 Changed 12 years ago by alberto

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

Ooops, I comitted a very similar patch myself at [2837] before looking at this ticket. Sorry :/

Alberto

Note: See TracTickets for help on using tickets.