-
Notifications
You must be signed in to change notification settings - Fork 315
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
fix: Fix DB unittest reliability #4548
Conversation
b3cfce7
to
fced295
Compare
@@ -248,21 +248,21 @@ dependencies = [ | |||
] | |||
|
|||
[tool.hatch.envs.default.scripts] | |||
tests = "pytest -n auto {args}" | |||
tests = "pytest {args}" |
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.
Do we no longer need pytest-xdist
?
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.
how many cores are we using if we don't specify this option? do we still need pytest-xdist
?
@@ -116,7 +111,7 @@ def openai_api_key(monkeypatch: pytest.MonkeyPatch) -> str: | |||
postgresql_connection = factories.postgresql("postgresql_proc") | |||
|
|||
|
|||
@pytest.fixture() | |||
@pytest.fixture(scope="function") |
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.
Isn't "function" the default scope?
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.
yes, but it's better to be explicit in case the default changes
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.
okay. fwiw i doubt pytest will change that default since it would be a very disruptive change.
@@ -248,21 +248,21 @@ dependencies = [ | |||
] | |||
|
|||
[tool.hatch.envs.default.scripts] | |||
tests = "pytest -n auto {args}" | |||
tests = "pytest {args}" |
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.
how many cores are we using if we don't specify this option? do we still need pytest-xdist
?
3f481ca
to
da69b6c
Compare
da69b6c
to
07b218a
Compare
* Remove busywait * Ruff 🐶 * Use closure loop * Use nest-asyncio for nested asgi fixture management * Wait for db insertions before reading in test * Ensure the entire experiment has run * Experiment with locks * xfail unstable tests * Use asyncio.sleep before querying database after client interactions * Ruff 🐶 * Reduce number of evaluators to make tests more reliable * Only bypass lock for unittests * Convert to an integration test * Set default loop scope for unit tests * Remove loop policy * xfail tests where evals do not reliably write to the database * Ensure databases are function scoped * Ensure inmemory sqlite testing * Ruff 🐶 * Wipe DBs between tests * Continue github actions on error * Use async sleep in spans test * Remove needless import * Refactor engine setup to potentially reduce deadlock risk * Wait for evaluations for more stable tests * Don't continue on failure * Ruff 🐶 * BulkInsterters insert immediately in tests * Remove xdist * Increase timeout to 30 * Xfail test * Use shared cache * Use tempfile based sqlite db * Use tempdirs for windows compatibility * Xfail test again * Wait a waiter to llm eval test * Skip flaky tests only on windows and mac
Updates our DB management strategy for unittests, this resolves the majority of stability issues we were seeing in CI.