-
Notifications
You must be signed in to change notification settings - Fork 53
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
Download example databases for use in CI tests #608
Conversation
Very interesting! I like this general direction. I do think we should avoid merging binary blobs like Taking in your comments on Slack, I was thinking about an approach like this to stitch it all together:
|
850b0c1
to
10cbf89
Compare
Notes from Zoom chat: Create a new repo,
Then, in Tiled, as with other tests that have external dependencies, make the test skippable, conditional on an env var being set, where the env var points to the URI of the database. Notably, this does not strictly require the database to be running in a container, but the container will be a convenient way to set this up. |
I have pushed commits refactoring the pytests fixtures.
As before, if
And now, if
TO DO:
|
Note that startup() and shutdown() are not called, and must be run | ||
either manually (as in the fixture 'a') or via the app (as in the fixture 'client'). |
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.
For ease of use, it seems like it would be convenient to keep startup()
and shutdown()
in the fixtures. Is it straightforward to add a guard to startup()
that checks whether it has already been called or whether an app is running?
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 do dislike this structure and would welcome suggestions to improve it.
The problem is that startup must be called on the same thread where the application will run. If this adapter is going to be used by the TestClient, via
Context.from_app(build_app(adapter)), a background thread is created at that point and
startup()` needs to be run on that thread.
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 see. In that case, I think the solution using the fixture a
is probably already optimal.
One could of course add fixture b
(or equivalent) for test_metadata_index_is_used(b)
-- if you expect additional tests that would make use of postgresql_with_example_data_adapter
:
@pytest_asyncio.fixture
async def b(postgresql_with_example_data_adapter):
"Raw adapter, not to be used within an app becaues it is manually started and stopped."
adapter = postgresql_with_example_data_adapter
await adapter.startup()
yield adapter
await adapter.shutdown()
However, a DRYer and more composable version might look like this...
@pytest.mark.parametrize("a", ["postgresql_with_example_data_adapter"], indirect=True)
@pytest.mark.asyncio
async def test_metadata_index_is_used(a):
# a, a.startup(), a.shutdown() are no longer needed
...
Marking the parameter as indirect
will override the argument passed to a parameterized fixture. See "Indirect parametrization" | parametrize
| and especially this informative example.
Continuing from the last PR I submitted to Tiled which switched postgresql indexing from
btree
tobtree_gin
indexes to support faster queries, an issue emerged when it came to running accurate index usage tests.The query planner will not use an index if a catalogue table contains fewer than 10,000 records. As a workaround, this PR looks to add a form of caching, via a container registry and the docker postgres image.
This is a work in progress to track changes and will contain numerous additional commits prior to any potential merge.