Clear search index *before* unittests #9653
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Move all the pytest fixtures defined in `tests/common/fixtures/*.py`
files into `conftest.py` files.
Context
-------
pytest provides a method of organising fixtures by putting them into
hierarchical/local `conftest.py` files:
tests/
conftest.py # Fixtures defined in this file are available to all tests.
unit/
conftest.py # Available to all unit tests.
functional/
conftest.py # Available to all functests.
foo/
conftest.py # Available to all tests in foo/ and below.
pytest automatically discovers and registers fixtures defined in
`conftest.py` files and makes them available to other files (both test
files and lower `conftest.py` files) for fixture injection _without
those files having to import the fixtures_.
See:
https://docs.pytest.org/en/stable/reference/fixtures.html#conftest-py-sharing-fixtures-across-multiple-files
A downside of this is that `conftest.py` files could get quite large but
it shouldn't get too bad: you shouldn't have too many shared fixtures
anyway.
At Hypothesis, [we thought we knew better](#5002).
We allowed this downside to tempt us to create
`tests/common/fixtures/*.py` files, allowing us to split our fixtures up
into multiple separate files. Our `conftest.py` files then needed to
import these fixtures from `tests.common.fixtures` so that pytest would
register them.
Problem
-------
Ah, hubris. In an upcoming future PR I need to have a common `es_client`
fixture that's shared between the unittests and the functests, and I
need to override that fixture to add some custom behaviour for the
unittests only. Here's how you'd normally do it with `conftest.py`
files, this works:
# tests/conftest.py
@pytest.fixture
def es_client():
client = _es_client()
yield client
client.close()
# tests/unit/conftest.py
@pytest.fixture
def es_client(es_client):
yield es_client
`tests/conftest.py` defines an `es_client` fixture that's available to
all test files and `conftest.py` files in the `tests/` directory, both
unittests and functests.
`tests/unit/conftest.py` then overrides the `es_client` fixture with its
own `es_client` fixture that depends on the higher-level `es_client`
fixture and adds some additional teardown.
This is normal pytest fixture overriding.
With our `tests/common/fixtures/` directory this doesn't work:
# tests/common/fixtures/elasticsearch.py
@pytest.fixture
def es_client():
client = _es_client()
yield client
client.close()
# tests/unit/conftest.py
from tests.common.fixtures.elasticsearch import es_client
@pytest.fixture
def es_client(es_client):
yield es_client
clear_search_index()
What happens is:
1. `tests/unit/conftest.py` has to import the `es_client` fixture from
`tests/common/fixtures/elasticsearch.py`, otherwise pytest won't
discover the fixture.
2. When `tests/unit/conftest.py` then defines its own `es_client`
function that overrides the name `es_client` in the module's
namespace, so now pytest will _not_ discover the original `es_client`
function that was imported from the common directory.
3. When pytest sees an `es_client` fixture that depends on a fixture
named `es_client`, it thinks the fixture is trying to depend on
itself and crashes with a circular fixture dependency error.
Solution
--------
Get rid of the `tests/common/fixtures/` directory. Just do the normal
pytest thing and move all these fixtures into `tests/conftest.py`. When
any other files (including lower `conftest.py` files) want to use any of
these common fixtures they can just do so via pytest fixture injection
without needing to import anything.
There were some fixtures in `tests/common/fixtures/services.py` that
were actually only used in the unittests. These have been moved into
`tests/unit/h/conftest.py`.
It *is* possible to have our cake and eat it here: you can put your
fixtures in separate files (thus avoiding have them all in one large
file) and then import them all into the top-level `tests/conftest.py`
file and all other files use the fixtures via injection. But I think
it's simpler just to put shared fixtures in `conftest.py` files.
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.
Clear the search index *before* each unittest rather than after, as this seems more robust. See: #9638 (comment)
seanh
force-pushed
the
clean-search-index-before-unittests
branch
from
c7ab862 to
929ed29
Compare
robertknight
approved these changes
Jun 24, 2025
seanh
force-pushed
the
clean-search-index-when-cleaning-db
branch
from
886a2bc to
3a3bd05
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clear the search index before each unittest rather than after, as this
seems more robust. See:
#9638 (comment)