Clear the functests search index when clearing the DB

[seanh]

For speed h does not normally clear the DB between functests unless the
test has @pytest.mark.parametrize("with_clean_db"), see:
#6845

At the time when this change was made the search index was still being
cleared before each functest which makes no sense: the search API will
not return an annotation if it's not in the search index, even if the
annotation is in the DB. If you aren't going to clear the DB before each
functest, you shouldn't clear the search index either.

A later PR accidentally broke the
always_delete_all_elasticsearch_documents fixture so that the search
index wasn't actually being cleared between functests anyway, see:
#9636

This commit:

1. Removes the broken always_delete_all_elasticsearch_documents
   fixture: it doesn't do anything, and since we don't clear the DB
   between tests it doesn't make sense to clear the search index either.
2. Replaces the with_clean_db fixture with
   with_clean_db_and_search_index: it doesn't make sense to clear the
   DB without also clearing the search index.

Longer-term I think we probably want to find a fast way to clear both
the DB and search index before each functional test.

[seanh]

The global es_client fixture (common to both unittests and functests) no longer clears the Elasticsearch index after each test. This has been moved into a unittests-only fixture.

[seanh]

The with_clean_db() functests fixture has been replaced with a with_clean_db_and_search_index() fixture: it doesn't make sense for a test to request a clean DB but not a clean search index. You can only have neither or both, not one without the other.

[seanh]

As discussed in another PR (#9636, which I'm not planning to merge) this fixture is broken and doesn't do anything. The functionality that this fixture was supposed to provide has been moved into the with_clean_db_and_search_index fixture and is executed only for tests that use that fixure, not for every test.

[seanh]

This preserves the previous behaviour of the unit tests: they always clear the search index after every test.

[robertknight]

This clears the search index after the test runs. My understanding of yield fixtures is that the teardown code is still run if the test fails, but there is potential that it doesn't get run if the test process were aborted for some reason. I suspect this is rare enough not to be a problem, but I'll note it as a minor hazard.

[seanh]

Yeah. This is preserving existing behaviour, but I agree it seems more robust to clear the search index before a test rather than after. For some reason cleaning up after a test seems to be the common and recommended approach. This seems to be a harmless change to make so I've sent a follow-up PR for it: #9653

[robertknight]

At the time when this change was made the search index was still being
cleared before each functest which makes no sense: the search API will
not return an annotation if it's not in the search index, even if the
annotation is in the DB.

If we're merely not cleaning the DB as an optimization, then some tests might be OK with having "orphaned" pre-existing entries which will never be found by search. Still, this also creates potentially surprising results. I agree it would be best to find a fast way of clearing both.

[robertknight]

This clears the search index after the test runs. My understanding of yield fixtures is that the teardown code is still run if the test fails, but there is potential that it doesn't get run if the test process were aborted for some reason. I suspect this is rare enough not to be a problem, but I'll note it as a minor hazard.

[seanh]

I agree it would be best to find a fast way of clearing both.

IIRC the problem is that the unittests use a DB session that's specially wrapped in a transaction that's rolled back after each test:

h/tests/conftest.py

Lines 18 to 43 in 1615569

	@pytest.fixture
	def db_session(db_engine, db_sessionfactory):
	"""
	Return the SQLAlchemy database session.
	This returns a session that is wrapped in an external transaction that is
	rolled back after each test, so tests can't make database changes that
	affect later tests. Even if the test (or the code under test) calls
	session.commit() this won't touch the external transaction.
	Recipe adapted from:
	https://docs.sqlalchemy.org/en/20/orm/session_transaction.html#joining-a-session-into-an-external-transaction-such-as-for-test-suites
	"""
	connection = db_engine.connect()
	transaction = connection.begin()
	session = db_sessionfactory(
	bind=connection, join_transaction_mode="create_savepoint"
	)
	try:
	yield session
	finally:
	session.close()
	transaction.rollback()
	connection.close()

But in the functests there are two separate DB sessions: the one used by the tests themselves (to create test data, or do DB queries for test assertions) and then the one that the app creates for itself. This means that the tests need to actually commit any test data to the DB and this needs to be actually committed (not held in some transaction) because the data needs to be seen by the app which is using an entirely separate session. Similarly the app needs to actually commit any data changes it makes so that test assertions will see it. And wiping actually committed stuff from the DB after each test is much slower than rolling back a transaction that was never really committed. Since functests never call the app directly (only via WebTest) there's no way for functests to pass a shared and specially constructed DB session into the app like the unittests do. The test code and application code are actually running in a single shared process. But WebTest didn't appear to provide any way for functests to pass an arbitrary Python object like a DB session through to the app.

[seanh]

Rebased