-
Notifications
You must be signed in to change notification settings - Fork 442
Move tests/common/fixtures/*
into conftest.py
#9640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
7af1d56
to
0fe7f0d
Compare
"es.url": os.environ["ELASTICSEARCH_URL"], | ||
"es.index": os.environ["ELASTICSEARCH_INDEX"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the ELASTICSEARCH_INDEX
and ELASTICSEARCH_URL
constants as having to pass them around/import them into different files is a pain. Any file that wants these envvars can just read them directly from os.environ
.
@@ -35,6 +35,7 @@ setenv = | |||
dev: H_API_AUTH_COOKIE_SALT = {env:H_API_AUTH_COOKIE_SALT:"dev_h_api_auth_cookie_salt"} | |||
dev: REPLICA_DATABASE_URL = {env:DATABASE_URL:postgresql://postgres@localhost/postgres} | |||
dev: MAILCHIMP_USER_ACTIONS_SUBACCOUNT = {env:MAILCHIMP_USER_ACTIONS_SUBACCOUNT:devdata} | |||
tests,functests: ELASTICSEARCH_URL = {env:ELASTICSEARCH_URL:http://localhost:9200} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code that created the ELASTICSEARCH_URL
constant was defaulting it to localhost:9200
if the envvar didn't exist. I've moved that default value into tox.ini
as we do for other envvar default values. This means the code can assume the envvar always exists.
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.
0fe7f0d
to
b05436b
Compare
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:pytest automatically discovers and registers fixtures defined in
conftest.py
files and makes them available to other files (both testfiles and lower
conftest.py
files) for fixture injection withoutthose 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 butit shouldn't get too bad: you shouldn't have too many shared fixtures
anyway.
At Hypothesis, we thought we knew better.
We allowed this downside to tempt us to create
tests/common/fixtures/*.py
files, allowing us to split our fixtures upinto multiple separate files. Our
conftest.py
files then needed toimport these fixtures from
tests.common.fixtures
so that pytest wouldregister them.
Problem
Ah, hubris. In an upcoming future PR (#9638) 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
defines anes_client
fixture that's available toall test files and
conftest.py
files in thetests/
directory, bothunittests and functests.
tests/unit/conftest.py
then overrides thees_client
fixture with itsown
es_client
fixture that depends on the higher-leveles_client
fixture and adds some additional teardown.
This is normal pytest fixture overriding.
With our
tests/common/fixtures/
directory this doesn't work:What happens is:
tests/unit/conftest.py
has to import thees_client
fixture fromtests/common/fixtures/elasticsearch.py
, otherwise pytest won'tdiscover the fixture.
tests/unit/conftest.py
then defines its ownes_client
function that overrides the name
es_client
in the module'snamespace, so now pytest will not discover the original
es_client
function that was imported from the common directory.
es_client
fixture that depends on a fixturenamed
es_client
, it thinks the fixture is trying to depend onitself and crashes with a circular fixture dependency error.
Solution
Get rid of the
tests/common/fixtures/
directory. Just do the normalpytest thing and move all these fixtures into
tests/conftest.py
. Whenany other files (including lower
conftest.py
files) want to use any ofthese 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
thatwere 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.