Clear the functests search index when clearing the DB

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.

Author: seanh

tests/conftest.py

@@ -57,25 +57,36 @@ def db_session_replica(db_session):
 @pytest.fixture
 def es_client():
     client = _es_client()
+
     yield client
-    # Push all changes to segments to make sure all annotations that were added get removed.
-    elasticsearch_dsl.Index(client.index, using=client.conn).refresh()
-
-    client.conn.delete_by_query(
-        index=client.index,
-        body={"query": {"match_all": {}}},
-        # This query occasionally fails with a version conflict.
-        # Forcing the deletion resolves the issue, but the exact
-        # cause of the version conflict has not been found yet.
-        conflicts="proceed",
-        # Add refresh to make deletion changes show up in search results.
-        refresh=True,
-    )
 
     # Close connection to ES server to avoid ResourceWarning about a leaked TCP socket.
     client.close()
 
 
+@pytest.fixture
+def clear_search_index(es_client):
+    """Return a function that deletes all documents from the search index."""
+
+    def clear_search_index():
+        """Delete all documents from the Elasticsearch index."""
+        # Push all changes to segments to make sure all annotations that were added get removed.
+        elasticsearch_dsl.Index(es_client.index, using=es_client.conn).refresh()
+
+        es_client.conn.delete_by_query(
+            index=es_client.index,
+            body={"query": {"match_all": {}}},
+            # This query occasionally fails with a version conflict.
+            # Forcing the deletion resolves the issue, but the exact
+            # cause of the version conflict has not been found yet.
+            conflicts="proceed",
+            # Add refresh to make deletion changes show up in search results.
+            refresh=True,
+        )
+
+    return clear_search_index
+
+
 @pytest.fixture
 def mock_es_client(es_client):
     return create_autospec(

tests/functional/api/bulk/action_test.py

@@ -10,7 +10,7 @@
 pytestmark = pytest.mark.usefixtures("init_elasticsearch")
 
 
-@pytest.mark.usefixtures("with_clean_db")
+@pytest.mark.usefixtures("with_clean_db_and_search_index")
 class TestBulk:
     AUTHORITY = "lms.hypothes.is"
 

tests/functional/api/bulk/annotation_test.py

@@ -5,7 +5,7 @@
 from h.models import GroupMembership
 
 
-@pytest.mark.usefixtures("with_clean_db")
+@pytest.mark.usefixtures("with_clean_db_and_search_index")
 class TestBulkAnnotation:
     def test_it_requires_authentication(self, make_request):
         response = make_request(headers={"bad_auth": "BAD"}, expect_errors=True)

tests/functional/bin/run_data_task_test.py

@@ -11,7 +11,7 @@
 
 class TestRunSQLTask:
     # We use "clean DB" here to ensure the schema is created
-    @pytest.mark.usefixtures("with_clean_db")
+    @pytest.mark.usefixtures("with_clean_db_and_search_index")
     def test_reporting_tasks(self, environ):
         for task_name in (
             # Run all the jobs in one line in the order they are expected to

tests/functional/conftest.py

@@ -1,6 +1,7 @@
 import contextlib
 import os
 
+import elasticsearch.exceptions
 import pytest
 from sqlalchemy import text
 from webtest import TestApp
@@ -51,7 +52,18 @@ def reset_app(app):
 
 
 @pytest.fixture
-def with_clean_db(db_engine):
+def with_clean_db_and_search_index(db_engine, clear_search_index):
+    """Empty the DB and search index before running the test.
+
+    h doesn't normally reset the DB or search index before each functest
+    because doing so was taking too long, see:
+    https://github.com/hypothesis/h/pull/6845
+
+    This does mean that functests need to be robust against data from previous
+    tests still being in the DB and search index. Alternatively, a test can use
+    @pytest.mark.usefixtures("with_clean_db_and_search_index") to have the DB
+    and search index emptied before running the test.
+    """
     tables = reversed(db.Base.metadata.sorted_tables)
     with contextlib.closing(db_engine.connect()) as conn:
         tx = conn.begin()
@@ -65,6 +77,12 @@ def with_clean_db(db_engine):
     db.Base.metadata.create_all(db_engine)
     db.post_create(db_engine)
 
+    # A test may not use the search index but may nonetheless use this
+    # fixture because it wants a clean DB. In that case trying to clear the
+    # search index will get a NotFoundError. Just suppress it.
+    with contextlib.suppress(elasticsearch.exceptions.NotFoundError):
+        clear_search_index()
+
 
 @pytest.fixture
 def db_session(db_engine, db_sessionfactory):
@@ -84,10 +102,3 @@ def factories(db_session):
 @pytest.fixture(scope="session")
 def pyramid_app():
     return create_app(None, **TEST_SETTINGS)
-
-
-# Always unconditionally wipe the Elasticsearch index after every functional
-# test.
-@pytest.fixture(autouse=True)
-def always_delete_all_elasticsearch_documents():
-    pass

tests/unit/h/conftest.py

@@ -67,6 +67,12 @@
 from tests.common import factories as common_factories
 
 
+@pytest.fixture
+def es_client(es_client, clear_search_index):
+    yield es_client
+    clear_search_index()
+
+
 class DummyFeature:
     """
     A dummy feature flag looker-upper.