ckan/ckan-service-provider — pull request #22 by seanh (2014-11-17 11:45:16+00:00)
Title: Get the tests passing reliably

Get the tests passing

The ckanserviceprovider tests are non-deterministic and usually don't pass.
This commit makes the minimal changes necessary to make the tests
deterministic and get them passing reliably.

Problems with the tests:

- Test ordering: some tests depend on other tests, will fail if the other tests
fail or are modified. The tests have been made independent by resetting the
db, scheduler, test client etc between tests

- Large tests that test several things at once and should be broken into small
tests. Some of these have been broken where it was necessary to break race
conditions to get the tests passing, there are many more that should be
broken up.

- Race conditions within tests: where a test kicks off multiple asynchronous
background jobs, waits for them to complete, then asserts stuff but the jobs
race each other and do not necessarily finish in the order that the test
asserts expect them to. These are fixed by breaking tests that start multiple
jobs up into separate tests.

- Race conditions between tests: sometimes an asynchronous job started by one
test finishes while a later test is running and causes asserts in the later
test to fail. This is fixed by resetting the scheduler and db between tests.

- Because of the way Flask and ckanserviceprovider work it's not possible to
reset the database between tests. Code related to initialising the Flask app,
db and scheduler in web.py and db.py had to be refactored to make this
possible.

There is much more to be done both in refactoring the tests (in particular get
rid of time.sleep(0.2) as a way to wait for asynchronous jobs to finish, which
is a race condition) and the ckanserviceprovider code itself, but it seems a
good idea to get this first step merged first.

Changes:

Refactor db.py. These are the first steps in making db.py into a proper
database module that encapsulates all the database code: provides a set of
simple unit-tested functions for working with the db, other modules call these
functions and don't do their own sqlalchemy:

- Add init() function that initialises the sqlalchemy engine object that we
use to connect to the db, creates database and tables if they don't exist

- Add drop_all() function that deletes the tables, for tests

- Add add_pending_job() function for adding a job to the tables.
Refactor web.py code to use this function instead of its own sqlalchemy.
Tests can now call this convenient function as well, when they need some jobs
in the db to test against.

- Add mark_job_as_completed() function for marking a job in the db as complete.
Tests can now call this function when they need completed jobs to test
against.
web.py code has not been refactored to use this yet - it's too complicated,
needs to be done as a separate commit.

Refactor web.py:

- Add init() function that initialises everything, to be called once at
application or tests startup. This cannot be called once per test or Flask's
login manager plugin crashes.

- Add a separate init_scheduler() function to initialise apscheduler,
this is separate because it must be called once per test

- Call db.add_pending_job() instead of own sqlalchemy

- Add main() function that calls init()

Refactor test_web.py:

- Add configure() function that calls web.init() once at module load time
(to initialise the Flask app)

- Add a reset_db() function that resets the db and scheduler, called after each
test method. By resetting the db and scheduler between tests we remove
dependencies and race conditions between tests.

- Make login() a function (doesn't need to be a method)

- Remove logout() as it doesn't seem to be needed (each test method initialises
a new test client object)

- Add test_client() function for each test method to call to get a test client
object for the flask app. Let each test method call this itself if it needs
to, rather than using a self.app object and potentially sharing state
between tests

- Split the huge test_asynchronous_post() method into many smaller tests.
This removes a race condition between multiple asynchronous jobs in the test
that was making the tests non-deterministic - by moving each job into its
own test they no longer race each other.
A lot more breaking down test methods could be done.

- The test_z_test_list() function (now renamed test_list()) now has to create
its own test data, since it can no longer rely on data put into the db by
previous tests

- The test_zz_test_clear_all() function (now renamed test_clear_all()) now has
to create its own test data, since it can no longer rely on data put into the
db by previous tests

- Other minor fixes to make tests pass