Async parity and unlock scope #1235
Conversation
|
Warning Rate limit exceeded@ivanthewebber has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds comprehensive asynchronous support: async fixtures, AsyncDatabaseJanitor, async loader and retry utilities, fixture-scope configurability, optional async dependencies, and corresponding async test coverage across the test suite. Changes
Sequence Diagram(s)sequenceDiagram
actor Test
participant Fixture as postgresql_async
participant Janitor as AsyncDatabaseJanitor
participant DB as PostgreSQL (AsyncConnection)
Test->>Fixture: request fixture (scope)
activate Fixture
Fixture->>Janitor: instantiate Janitor
activate Janitor
Janitor->>DB: connect (AsyncConnection)
activate DB
Janitor->>Janitor: init() / create DB
Janitor->>Janitor: load() via build_loader_async / sql_async
Fixture->>Test: yield AsyncConnection
deactivate DB
deactivate Janitor
Test->>Fixture: test completes
activate Fixture
Fixture->>Janitor: trigger drop()/cleanup
activate Janitor
Janitor->>DB: close connection
deactivate Janitor
deactivate Fixture
sequenceDiagram
participant Caller as test/code
participant Loader as build_loader_async
participant SQL as sql_async
participant Retry as retry_async
participant DB as AsyncConnection
Caller->>Loader: build_loader_async(path | "module.attr" | callable)
alt Path input
Loader-->>Caller: partial(sql_async, path)
else Str import path
Loader->>Loader: importlib.import_module(...)
Loader-->>Caller: resolved callable
else Callable
Loader-->>Caller: passthrough callable
end
Caller->>Retry: retry_async(loader_func, timeout)
activate Retry
loop until success or timeout
Retry->>Loader: await loader_func()
alt sql_async used
Loader->>SQL: execute async SQL file
SQL->>DB: run statements
DB-->>SQL: results
else callable
Loader-->>Retry: result / raise
end
alt exception
Retry->>Retry: await asyncio.sleep(1)
else success
Retry-->>Caller: return result
end
end
deactivate Retry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (8)
pyproject.toml (1)
39-42: Consider adding a compatible lower bound forpytest-asyncio, and ensure CI installs the extra.Right now
async = ["pytest-asyncio"]leaves version selection unconstrained; it’s safer to set a minimum known to work with your supportedpytest/Python versions, and to ensure the test environment installs.[async](or equivalent dev deps).pytest_postgresql/factories/__init__.py (1)
20-24: Decide whetherpostgresql_asyncis public; if yes, add it to__all__.If users are meant to consume
pytest_postgresql.factories.postgresql_async, exporting it in__all__keeps API parity withpostgresql,postgresql_proc, etc.pytest_postgresql/retry.py (1)
40-66:retry_asynclooks correct; consider monotonic time for timeout robustness.Using
datetimefor timeouts can misbehave if system time changes;time.monotonic()(orasyncio.get_running_loop().time()) avoids that.pytest_postgresql/factories/noprocess.py (1)
25-26: Avoid_pytest.scope._ScopeName(private pytest API) for public typing.Consider replacing the import with a local
typing.Literal["function", "class", "module", "package", "session"]alias to reduce coupling to pytest internals.Also applies to: 49-50, 60-65
tests/docker/test_noproc_docker.py (1)
61-73: Add type annotation for consistency.The
async_postgres_with_templateparameter lacks a type annotation, unlike other async test functions in this file.@pytest.mark.asyncio @pytest.mark.parametrize("_", range(5)) -async def test_template_database_async(async_postgres_with_template, _: int) -> None: +async def test_template_database_async(async_postgres_with_template: AsyncConnection, _: int) -> None: """Async check that the database structure gets recreated out of a template."""pytest_postgresql/janitor.py (3)
214-226: Async init/drop parity looks good; consider using identifiers instead of f-strings. The logic mirrors the sync janitor well, but interpolating database names via f-strings is brittle (quoting edge-cases) and can become injection-prone if names ever come from user config. Consider psycopg’s SQL composition (psycopg.sql.Identifier) for database identifiers.Also applies to: 231-241
300-305: Add return types for__aenter__/__aexit__(API polish). This improves editor support and keeps parity with the typed sync context manager.@@ - async def __aenter__(self): + async def __aenter__(self) -> "AsyncDatabaseJanitor": @@ - async def __aexit__(self, exc_type, exc_val, exc_tb): + async def __aexit__(self, exc_type, exc_val, exc_tb) -> None:
255-276: Renamecortoresultfor clarity. The variable namecorsuggests a coroutine, butbuild_loader_async()can return callables that execute synchronously and returnNone(when a sync function is provided) or return an awaitable (when an async function orpartial(sql_async, ...)is provided). The current await logic correctly handles both cases withinspect.isawaitable(), but renaming toresultbetter reflects that the value could be any return type, not necessarily a coroutine.@@ - cor = _loader( + result = _loader( @@ - if inspect.isawaitable(cor): - await cor + if inspect.isawaitable(result): + await result
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
pyproject.toml(1 hunks)pytest_postgresql/factories/__init__.py(1 hunks)pytest_postgresql/factories/client.py(3 hunks)pytest_postgresql/factories/noprocess.py(3 hunks)pytest_postgresql/factories/process.py(3 hunks)pytest_postgresql/janitor.py(3 hunks)pytest_postgresql/loader.py(3 hunks)pytest_postgresql/retry.py(2 hunks)tests/conftest.py(1 hunks)tests/docker/test_noproc_docker.py(4 hunks)tests/test_janitor.py(5 hunks)tests/test_loader.py(2 hunks)tests/test_postgresql.py(2 hunks)tests/test_template_database.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
tests/conftest.py (1)
pytest_postgresql/factories/client.py (1)
postgresql_async(93-159)
tests/test_postgresql.py (3)
pytest_postgresql/factories/client.py (1)
postgresql_async(93-159)pytest_postgresql/janitor.py (2)
cursor(131-152)cursor(278-298)pytest_postgresql/retry.py (1)
retry(12-37)
tests/docker/test_noproc_docker.py (1)
pytest_postgresql/factories/client.py (2)
postgresql_async(93-159)postgresql(34-90)
pytest_postgresql/factories/client.py (2)
pytest_postgresql/janitor.py (8)
AsyncDatabaseJanitor(169-305)DatabaseJanitor(22-166)drop(84-95)drop(231-240)connect(134-141)connect(281-288)load(110-128)load(255-275)pytest_postgresql/config.py (1)
get_config(28-52)
pytest_postgresql/loader.py (1)
pytest_postgresql/janitor.py (2)
load(110-128)load(255-275)
tests/test_loader.py (2)
pytest_postgresql/loader.py (4)
build_loader(12-24)build_loader_async(36-48)sql(27-33)sql_async(51-57)tests/loader.py (1)
load_database(9-18)
tests/test_template_database.py (2)
tests/docker/test_noproc_docker.py (1)
test_template_database(49-58)pytest_postgresql/janitor.py (2)
cursor(131-152)cursor(278-298)
🪛 Ruff (0.14.8)
tests/test_janitor.py
74-74: Possible hardcoded password assigned to argument: "password"
(S106)
78-78: Possible hardcoded password assigned to argument: "password"
(S106)
pytest_postgresql/retry.py
61-61: Consider moving this statement to an else block
(TRY300)
64-64: Avoid specifying long messages outside the exception class
(TRY003)
pytest_postgresql/factories/client.py
153-153: Undefined name pg_load
(F821)
tests/test_loader.py
23-23: Unused function argument: args
(ARG001)
23-23: Unused function argument: kwargs
(ARG001)
tests/test_template_database.py
36-36: Redefinition of unused test_template_database from line 23
(F811)
36-36: Unused function argument: async_postgresql_template
(ARG001)
🔇 Additional comments (15)
tests/test_template_database.py (1)
4-4: AsyncConnection import is fine for psycopg v3 async coverage.tests/test_loader.py (2)
5-8: Async loader test imports are straightforward.
37-43: Async SQL loader mapping coverage is good.tests/docker/test_noproc_docker.py (3)
17-17: LGTM!The async fixture is correctly defined using
postgresql_asyncfactory, mirroring the synchronous counterpart.
25-27: LGTM!Async fixture for template database correctly configured with dbname parameter.
40-46: LGTM!Async test correctly uses
async withfor cursor context management and awaits all async operations.tests/test_janitor.py (4)
22-27: LGTM!Async version cast test correctly mirrors the synchronous test and validates
AsyncDatabaseJanitorversion handling.
38-44: LGTM!Async cursor test properly mocks
AsyncConnection.connectand validates the expected database connection parameters.
64-79: LGTM!Async password connection test correctly validates that password is passed through to the connection. The hardcoded password is appropriate for test fixtures.
103-119: LGTM!Async populate test correctly validates the loader mechanism with both dot and colon separator formats for import paths.
tests/test_postgresql.py (4)
77-82: LGTM!Async main postgres test correctly uses async context manager and awaits cursor operations.
85-94: LGTM!Async two postgres test correctly validates multiple async connections in a single test.
97-103: LGTM!Async load test correctly verifies fixture loading with async cursor operations.
106-109: LGTM!Async port test correctly validates connection status.
pytest_postgresql/factories/client.py (1)
34-50: LGTM!The scope parameter addition to the synchronous fixture factory is clean and correctly applies the scope to the pytest fixture decorator.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tests/test_template_database.py (1)
40-51: Missing@pytest.mark.xdist_groupdecorator for parallel test coordination.The synchronous test at line 26 has
@pytest.mark.xdist_group(name="template_database")to prevent parallel interference when running with pytest-xdist. The async test should have the same marker to ensure both tests don't run simultaneously and interfere with each other on the shared template database.+@pytest.mark.xdist_group(name="template_database") @pytest.mark.asyncio @pytest.mark.parametrize("_", range(5)) async def test_template_database_async(async_postgresql_template: AsyncConnection, _: int) -> None:pytest_postgresql/loader.py (1)
44-44: Same incorrectimportlib.import_moduleusage in async variant.Apply the same fix as the synchronous version.
- _temp_import = importlib.import_module(import_path, loader_name) + _temp_import = importlib.import_module(import_path)pytest_postgresql/janitor.py (2)
40-51: Docstring parameter name fortemplate_dbnameis still incorrect in both janitorsThe constructor docstrings list
:param dbname:twice; the second entry in each class should describetemplate_dbname, notdbname. This was raised before for the async janitor and is still outstanding.You can align both docstrings like this:
@@ class DatabaseJanitor: - :param dbname: database name - :param dbname: template database name + :param dbname: database name + :param template_dbname: template database name @@ class AsyncDatabaseJanitor: - :param dbname: database name - :param dbname: template database name + :param dbname: database name + :param template_dbname: template database nameAlso applies to: 187-197
231-241: Fix potential connection leak inAsyncDatabaseJanitor.cursorifconn.cursor()failsAs written,
await conn.close()is inside theasync with conn.cursor()block. Ifconn.cursor()or entering the async context raises, thefinallyblock is never reached and the connection is left open.Restructuring to always close the connection, regardless of where an error occurs, would make this safer:
- conn = await retry_async(connect, timeout=self._connection_timeout, possible_exception=psycopg.OperationalError) - await conn.set_isolation_level(self.isolation_level) - await conn.set_autocommit(True) - # We must not run a transaction since we create a database. - async with conn.cursor() as cur: - try: - yield cur - finally: - await conn.close() + conn = await retry_async( + connect, + timeout=self._connection_timeout, + possible_exception=psycopg.OperationalError, + ) + try: + await conn.set_isolation_level(self.isolation_level) + await conn.set_autocommit(True) + # We must not run a transaction since we create a database. + async with conn.cursor() as cur: + yield cur + finally: + await conn.close()This follows the same intent but guarantees that the underlying async connection is closed even if cursor acquisition or context entry fails.
Also applies to: 277-305
🧹 Nitpick comments (1)
pyproject.toml (1)
39-43: Consider adding version constraints to optional dependencies.The async optional dependencies lack version constraints, which could lead to compatibility issues with future breaking changes. Consider specifying minimum versions, similar to the core dependencies.
[project.optional-dependencies] async = [ - "pytest-asyncio", - "aiofiles" + "pytest-asyncio >= 0.21", + "aiofiles >= 23.0" ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pyproject.toml(1 hunks)pytest_postgresql/factories/client.py(2 hunks)pytest_postgresql/factories/noprocess.py(3 hunks)pytest_postgresql/factories/process.py(3 hunks)pytest_postgresql/janitor.py(4 hunks)pytest_postgresql/loader.py(3 hunks)pytest_postgresql/types.py(1 hunks)tests/test_postgresql.py(2 hunks)tests/test_template_database.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pytest_postgresql/factories/process.py
🧰 Additional context used
🧬 Code graph analysis (3)
pytest_postgresql/loader.py (1)
pytest_postgresql/janitor.py (2)
load(110-128)load(255-275)
tests/test_postgresql.py (3)
pytest_postgresql/retry.py (2)
retry(12-37)retry_async(40-65)pytest_postgresql/factories/client.py (1)
postgresql_async(93-150)pytest_postgresql/janitor.py (2)
cursor(131-152)cursor(278-298)
pytest_postgresql/factories/client.py (1)
pytest_postgresql/janitor.py (6)
AsyncDatabaseJanitor(169-305)DatabaseJanitor(22-166)drop(84-95)drop(231-240)connect(134-141)connect(281-288)
🪛 Ruff (0.14.8)
pytest_postgresql/janitor.py
57-57: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
pytest_postgresql/types.py (1)
1-3: LGTM!Clean type alias definition covering all valid pytest fixture scopes. This provides good type safety for the scope parameter across factory modules.
pytest_postgresql/factories/noprocess.py (1)
30-30: LGTM!The scope parameter implementation is clean and maintains backward compatibility with the "session" default. The type annotation using
FixtureScopeTprovides proper type safety.Also applies to: 49-49, 64-64
pytest_postgresql/loader.py (1)
51-59: Async SQL loader implementation looks correct.The
aiofilesimport is appropriately placed inside the function to avoid import errors when the async optional dependency isn't installed. The cursor handling and awaits are correct for psycopg 3'sAsyncConnection.tests/test_postgresql.py (1)
77-109: Async test implementations look correct.All async tests properly use
@pytest.mark.asyncio, correctly await async operations, and maintain parity with their synchronous counterparts.pytest_postgresql/factories/client.py (2)
34-51: Configurable scope for the synchronous PostgreSQL fixture looks soundThreading the
scopeargument through to@pytest.fixturewhile keeping the default"function"preserves existing behaviour and lets users opt into class/session-level fixtures without extra boilerplate. No functional issues spotted in the way the scope type and default are wired here.
93-150: Async PostgreSQL fixture factory is correctly structured and mirrors the sync pathThe async factory correctly uses
pytest_asyncio.fixture,AsyncDatabaseJanitoras an async context manager, andAsyncConnection.connect(...)withawait, and ensures the connection is closed withawait db_connection.close(). This gives good parity with the synchronouspostgresqlfactory while keepingpytest_asyncioan optional import.pytest_postgresql/janitor.py (2)
172-213: AsyncDatabaseJanitor constructor validation and version handling look correctThe async constructor mirrors the synchronous one, validates that at least one of
dbnameortemplate_dbnameis provided via an explicitValueError, and normalisesversionto aVersioninstance. This is clear and robust and avoids the earlierassertpitfall.
255-276: Async loader integration looks reasonableUsing
build_loader_asyncandinspect.isawaitableto handle both synchronous and asynchronous loader callables is a pragmatic way to support mixed ecosystems and keeps the async surface area simple. No behavioural issues spotted here.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pytest_postgresql/janitor.py (1)
38-51: Fix duplicate parameter name in docstring.Line 44 has
:param dbname: template database namebut this should be:param template_dbname:to match the actual parameter.:param dbname: database name - :param dbname: template database name + :param template_dbname: template database name :param version: postgresql version number
♻️ Duplicate comments (1)
pytest_postgresql/janitor.py (1)
185-198: Fix duplicate parameter name in docstring.Line 191 has the same issue as the sync version -
:param dbname:appears twice instead of:param template_dbname:.:param dbname: database name - :param dbname: template database name + :param template_dbname: template database name :param version: postgresql version number
🧹 Nitpick comments (4)
pytest_postgresql/janitor.py (3)
84-95: Consider replacingassertwith explicitValueErrorfor consistency.Line 89 uses
assert db_to_dropwhich can be stripped withpython -O. For consistency with the__init__validation fix, consider using an explicit check.def drop(self) -> None: """Drop database in postgresql.""" db_to_drop = self.template_dbname if self.is_template() else self.dbname - assert db_to_drop + if not db_to_drop: + raise ValueError("No database to drop; either dbname or template_dbname must be set.") with self.cursor() as cur:
231-240: Consider replacingassertwith explicitValueErrorfor consistency.Line 234 uses
assert db_to_dropwhich has the same issue as the sync version.async def drop(self) -> None: """Drop database in postgresql (async).""" db_to_drop = self.template_dbname if self.is_template() else self.dbname - assert db_to_drop + if not db_to_drop: + raise ValueError("No database to drop; either dbname or template_dbname must be set.") async with self.cursor() as cur:
300-305: Consider adding type hints to async context manager methods.For consistency with the sync
DatabaseJanitorand better IDE support, consider adding type hints:- async def __aenter__(self): + async def __aenter__(self) -> "AsyncDatabaseJanitor": await self.init() return self - async def __aexit__(self, exc_type, exc_val, exc_tb): + async def __aexit__( + self, + exc_type: Type[BaseException] | None, + exc_val: BaseException | None, + exc_tb: TracebackType | None, + ) -> None: await self.drop()pytest_postgresql/loader.py (1)
51-59: Consider documenting the optionalaiofilesdependency in the function docstring or adding error handling.Although
aiofilesis declared as an optional dependency inpyproject.toml, thesql_asyncfunction lacks documentation about this requirement. If users call this function without installing the optional dependency, they will encounter a bareImportErrorrather than a clear message explaining that theasyncextra is required. Consider either adding a note to the docstring or wrapping the import with a try/except block that provides a more informative error message.The implementation is otherwise correct:
AsyncConnection.cursor()is correctly not awaited (synchronous factory in psycopg3), whilst I/O methods are properly awaited.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pytest_postgresql/janitor.py(4 hunks)pytest_postgresql/loader.py(3 hunks)tests/test_loader.py(2 hunks)tests/test_postgresql.py(2 hunks)tests/test_template_database.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_postgresql.py (3)
pytest_postgresql/retry.py (2)
retry(12-37)retry_async(40-65)pytest_postgresql/factories/client.py (1)
postgresql_async(93-150)pytest_postgresql/janitor.py (2)
cursor(131-152)cursor(278-298)
pytest_postgresql/janitor.py (3)
pytest_postgresql/loader.py (2)
build_loader(12-24)build_loader_async(36-48)pytest_postgresql/retry.py (2)
retry(12-37)retry_async(40-65)pytest_postgresql/executor.py (2)
template_dbname(135-137)version(201-213)
pytest_postgresql/loader.py (1)
pytest_postgresql/janitor.py (6)
load(110-128)load(255-275)connect(134-141)connect(281-288)cursor(131-152)cursor(278-298)
🪛 Ruff (0.14.8)
pytest_postgresql/janitor.py
57-57: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (9)
tests/test_loader.py (2)
17-27: LGTM!The async test correctly verifies
build_loader_asynchandles callable references, string import paths, and inline async functions. The unused parameter fix (*_args,**_kwargs) addresses the previous review comment.
37-43: LGTM!The async SQL loader test correctly verifies that
build_loader_asyncreturns a partial wrappingsql_asyncforPathinputs, mirroring the synchronous test structure.tests/test_template_database.py (2)
20-23: LGTM!The async fixture is correctly defined using
postgresql_asyncfactory, pointing to the same process fixture with the templated database name.
40-52: LGTM!The async test addresses all previously identified issues: unique function name, proper
async def, correct@pytest.mark.asynciodecorator, correct fixture parameter, and all necessaryawaitcalls. The separatexdist_groupname prevents parallel interference with the sync test.pytest_postgresql/loader.py (1)
36-48: LGTM!The async loader builder correctly mirrors the synchronous implementation, properly using
importlib.import_moduleand returning partials for Path inputs or pass-through for callables.tests/test_postgresql.py (2)
77-94: LGTM!The async tests correctly mirror their synchronous counterparts, properly using
async withfor cursor context management andawaitfor all I/O operations.
112-130: LGTM!The async connection termination test now includes the
@pytest.mark.skipifdecorator for PostgreSQL version checking (addressing the previous review comment), correctly usesretry_asyncinstead ofretry, and properly defines the inner check function asasync def.pytest_postgresql/janitor.py (2)
56-57: LGTM!The validation now correctly uses
ValueErrorinstead ofassert, ensuring it cannot be stripped by Python's optimisation flags (-O).
277-298: LGTM!The async cursor context manager correctly:
- Uses
retry_asyncfor connection retry logic- Uses async setters (
await conn.set_isolation_level,await conn.set_autocommit) per psycopg3 API- Wraps cursor usage in
try/finallyto ensure connection closure even if cursor creation fails- Does not await
conn.cursor()(correct - it's a synchronous factory in psycopg3)
|
@fizyk could you update |
fizyk
left a comment
fizyk
left a comment
There was a problem hiding this comment.
@ivanthewebber try this branch for setup-pipenv pipenv-categories
I'll release it once we'll get it running here. Haven't needed it anywhere else so far.
| unixsocketdir: str | None = None, | ||
| postgres_options: str | None = None, | ||
| load: list[Callable | str | Path] | None = None, | ||
| scope: FixtureScopeT = "session", |
There was a problem hiding this comment.
Why do you want to add the scope parameter to all the factories there are? What's the use case?
There was a problem hiding this comment.
For codebases that already have thousands of tests switching everything to a different scope to test out this library to see if it speeds up tests might be rather prohibitory. We have 6000 tests and over a hundred fixtures e.g. and I used this to get things running and next I can work on side effects of changing scope. Could have a warning or something if it's somehow critical
There was a problem hiding this comment.
Oh I see now; yes it's probably not necessary for proc and noproc.
There was a problem hiding this comment.
We have 6000 tests
😍 love it!
So, the original intention behind those scopes would be that:
process sets up the database instance (with later additon of template database)
And client fixture creates a one-off database and drops it after the test (with later addition of doing that based on the template). This makes sure the state does not leak between tests, and they do not influence each other or make the order of tests dependant.
I've tested it manually, and the speed is rather similar if I had created a single database and used transactions. However, then all your tests are inside transactions, and you have to use that single connection that set up the transaction (that's why warehouse is using just DatabaseJanitor in their tests).
With the template database approach, you can create your own client connection to read the state as you prepare it, and not worry about hanging transactions with data fixtures.
Even run external processes in tests, if you wish to do so.
To address somethig like you described, I planned #890, although got distracted along the way if thinking about it. Maybe it's time to return to that idea....
| pg_password = proc_fixture.password | ||
| pg_options = proc_fixture.options | ||
| pg_db = dbname or proc_fixture.dbname | ||
| janitor = AsyncDatabaseJanitor( |
There was a problem hiding this comment.
Do we really need AsyncDatabaseJanitor? Is there something in Janitor that would require tests to block on I/O?
This might come in handy in case test would have more async fixtures to prepare, not in a straightforward scenario with a single fixture?
There was a problem hiding this comment.
Your assessment is accurate; you can get around without it for sure unless you're using it directly in an async test or fixture.
2 participants
Please take the work from here, but these edits should add async support and unlock the fixture scope which will make adoption easier for companies like mine that are deeply entrenched in class scopes.
Chore that needs to be done:
pipenv run towncrier create [issue_number].[type].rstTypes are defined in the pyproject.toml, issue_number either from issue tracker or the Pull request number
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.