Conversation
WalkthroughAdds optional Python asyncio support (background task tracking and an autouse pytest fixture), refactors vcrpy host/recording handling, and strengthens CI pre-commit error reporting. Template prompts, copier resources, and test data gain flags and resources to opt into asyncio support. Changes
Sequence DiagramsequenceDiagram
actor Tester
participant Fixture as pytest autouse fixture
participant Tracker as background_tasks.register_task
participant BgTask as Background Task
participant EventLoop as Event Loop
Tester->>Tracker: register_task(task)
Tracker->>Tracker: capture creation traceback\nadd task to WeakSet\nattach done callback
Tester->>Fixture: test starts (fixture clears prior exceptions)
Tester->>EventLoop: run test (may spawn BgTask)
EventLoop->>BgTask: execute background work
BgTask-->>BgTask: completes or raises
BgTask->>Tracker: done callback records exceptions and cleans up
Tester->>Fixture: test ends (fixture resumes)
Fixture->>Tracker: collect pending tasks
Fixture->>EventLoop: await remaining tasks (5s timeout)
EventLoop-->>Fixture: tasks finished or timeout
Fixture->>Fixture: check recorded exceptions
alt exceptions recorded
Fixture->>Tester: fail test with details
else
Fixture->>Tester: allow test to pass
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/ci.yaml(1 hunks)copier.yaml(1 hunks)template/.github/workflows/ci.yaml.jinja-base(4 hunks)template/.github/workflows/pre-commit.yaml.jinja-base(1 hunks)template/copier.yml.jinja-base(1 hunks)template/copier_template_resources/{% if template_might_want_to_use_python_asyncio %}python_asyncio{% endif %}/asyncio_fixtures.py(1 hunks)template/copier_template_resources/{% if template_might_want_to_use_python_asyncio %}python_asyncio{% endif %}/background_tasks.py(1 hunks)template/copier_template_resources/{% if template_might_want_to_use_vcrpy %}vcrpy_fixtures.py{% endif %}(2 hunks)template/tests/copier_data/data1.yaml.jinja-base(1 hunks)template/tests/copier_data/data2.yaml.jinja-base(1 hunks)tests/copier_data/data1.yaml(1 hunks)tests/copier_data/data2.yaml(1 hunks)
🔇 Additional comments (16)
template/.github/workflows/pre-commit.yaml.jinja-base (1)
71-79: LGTM! Enhanced pre-commit error reporting.The multi-line shell script correctly captures the pre-commit exit code, displays diagnostic information, and shows the git diff when pre-commit fails. The bash logic using the
||operator ensuresPRE_COMMIT_EXIT_CODEis only set on failure, and the script properly preserves the original exit code for CI failure detection..github/workflows/ci.yaml (1)
62-70: LGTM! Consistent error handling across workflows.The enhanced pre-commit error reporting matches the pattern introduced in the template workflow, ensuring consistent diagnostic output across all CI workflows.
template/.github/workflows/ci.yaml.jinja-base (4)
23-24: LGTM! Improved git diff readability.The multi-line list format for job dependencies aligns with the PR objective to minimize git diffs when dependencies change.
33-34: LGTM! Consistent dependency format.The explicit list format maintains consistency with other
needsdeclarations in the workflow.
126-132: LGTM! Pre-commit error handling with hook skipping.The enhanced error reporting correctly handles the
SKIPenvironment variable prefix and follows the same robust error-handling pattern established in other workflows. The exit code capture and diff display will aid in debugging pre-commit failures during template instantiation.
144-146: LGTM! Multi-dependency declaration.The explicit list format clearly shows all job dependencies and will produce cleaner diffs when dependencies are added or removed.
tests/copier_data/data1.yaml (1)
15-15: LGTM!The new asyncio feature flag follows the established pattern for other template configuration options and correctly enables asyncio support for this test case.
tests/copier_data/data2.yaml (1)
17-17: LGTM!Good test coverage—having one test case with the flag enabled and another with it disabled ensures both conditional paths are exercised.
copier.yaml (1)
75-79: LGTM!The new asyncio configuration question follows the established pattern (similar to
template_might_want_to_use_vcrpy), is properly gated bytemplate_uses_python, and uses a conservative default.template/tests/copier_data/data2.yaml.jinja-base (1)
11-11: LGTM!The conditional block for
configure_python_asynciofollows the same pattern as the existingconfigure_vcrpyblock and is correctly positioned.template/tests/copier_data/data1.yaml.jinja-base (1)
11-11: LGTM!The conditional block correctly enables
configure_python_asynciofor this test case, providing complementary coverage to data2.yaml.jinja-base.template/copier.yml.jinja-base (1)
49-53: LGTM!The new
configure_python_asynciooption follows the established pattern. Also good to see thedefault: noadded toconfigure_vcrpyfor consistency.template/copier_template_resources/{% if template_might_want_to_use_python_asyncio %}python_asyncio{% endif %}/background_tasks.py (3)
1-13: LGTM!Good design choices:
WeakSetprevents memory leaks from lingering task referencesdequewithmaxlen=100prevents unbounded growth in production- Storing creation tracebacks in a dict enables helpful debugging context
16-32: Well-structured exception handling.The done callback properly differentiates between cancellation, exceptions, and successful completion. The creation traceback logging on Line 28 will be valuable for debugging background task failures.
35-41: LGTM!The
register_taskfunction correctly captures the creation stack trace (excluding its own frame with[:-1]) and wires up the cleanup callback.template/copier_template_resources/{% if template_might_want_to_use_vcrpy %}vcrpy_fixtures.py{% endif %} (1)
8-18: The current implementation is correct; no refactor needed.The concern about
allowed_hosts: []being semantically different from omitting the key is unfounded. In pytest-recording, an emptyallowed_hostslist is functionally equivalent to omitting the key entirely—both use the default behavior where no additional hosts are whitelisted through the network blocker. The VCR config'signore_hostssetting (currentlyIGNORED_HOSTSandCUSTOM_IGNORED_HOSTS) remains the mechanism controlling which hosts are allowed to make live requests.On non-Windows,
allowed_hosts: []poses no risk to the PR intent; on Windows,allowed_hostsis correctly populated with the required localhost addresses. The code does not need conditional emission of theallowed_hostskey.Likely an incorrect or invalid review comment.
| async def _wait_for_tasks(tasks_list: list[asyncio.Task[None]]): | ||
| _, pending = await asyncio.wait(tasks_list, timeout=5.0) | ||
| if pending: | ||
| raise RuntimeError(f"There are still pending tasks: {pending}") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making the timeout configurable.
The 5-second timeout is hardcoded, which might be too short for slow CI environments or unnecessarily long for local development. Consider making it configurable via an environment variable or fixture parameter.
Example:
+import os
+
+# At module level
+DEFAULT_TASK_TIMEOUT = float(os.getenv("ASYNCIO_TASK_TIMEOUT", "5.0"))
+
-async def _wait_for_tasks(tasks_list: list[asyncio.Task[None]]):
- _, pending = await asyncio.wait(tasks_list, timeout=5.0)
+async def _wait_for_tasks(tasks_list: list[asyncio.Task[None]], timeout: float = DEFAULT_TASK_TIMEOUT):
+ _, pending = await asyncio.wait(tasks_list, timeout=timeout)
if pending:
raise RuntimeError(f"There are still pending tasks: {pending}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def _wait_for_tasks(tasks_list: list[asyncio.Task[None]]): | |
| _, pending = await asyncio.wait(tasks_list, timeout=5.0) | |
| if pending: | |
| raise RuntimeError(f"There are still pending tasks: {pending}") | |
| import os | |
| # At module level | |
| DEFAULT_TASK_TIMEOUT = float(os.getenv("ASYNCIO_TASK_TIMEOUT", "5.0")) | |
| async def _wait_for_tasks(tasks_list: list[asyncio.Task[None]], timeout: float = DEFAULT_TASK_TIMEOUT): | |
| _, pending = await asyncio.wait(tasks_list, timeout=timeout) | |
| if pending: | |
| raise RuntimeError(f"There are still pending tasks: {pending}") |
| try: | ||
| loop = asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| asyncio.run(_wait_for_tasks(tasks_list)) | ||
| else: | ||
| loop.run_until_complete(_wait_for_tasks(tasks_list)) |
There was a problem hiding this comment.
Fix the event loop handling to support async test functions.
The logic on Line 29 attempts to call loop.run_until_complete() on an already-running event loop, which will raise RuntimeError: "This event loop is already running". This occurs when the fixture runs after an async test (e.g., when using pytest-asyncio).
The fixture cannot call run_until_complete on a running loop, nor can it await (since it's not async). You need either:
- An async version of this fixture for async tests
- A different mechanism that doesn't require executing async code from the fixture teardown
Consider this approach if pytest-asyncio is available:
+import pytest_asyncio
+
+# For async tests
+@pytest_asyncio.fixture(autouse=True)
+async def fail_on_background_task_errors_async():
+ """Automatically fail async tests if ANY background task raises an exception."""
+ background_task_exceptions.clear()
+
+ yield
+
+ if background_tasks_set:
+ tasks_list = list(background_tasks_set)
+ await _wait_for_tasks(tasks_list)
+
+ if background_task_exceptions:
+ pytest.fail(
+ f"Background tasks raised {len(background_task_exceptions)} exception(s):\n"
+ + "\n\n".join(f"{type(e).__name__}: {e}" for e in background_task_exceptions)
+ )
+
+# For sync tests
@pytest.fixture(autouse=True)
def fail_on_background_task_errors():
"""Automatically fail tests if ANY background task raises an exception."""
background_task_exceptions.clear()
yield
- # Wait for background tasks to complete (using asyncio.run for sync fixture)
+ # Wait for background tasks to complete (only for sync tests)
if background_tasks_set:
tasks_list = list(background_tasks_set)
- try:
- loop = asyncio.get_running_loop()
- except RuntimeError:
- asyncio.run(_wait_for_tasks(tasks_list))
- else:
- loop.run_until_complete(_wait_for_tasks(tasks_list))
+ asyncio.run(_wait_for_tasks(tasks_list))
# Fail if any exceptions occurred
if background_task_exceptions:
pytest.fail(
f"Background tasks raised {len(background_task_exceptions)} exception(s):\n"
+ "\n\n".join(f"{type(e).__name__}: {e}" for e in background_task_exceptions)
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| loop = asyncio.get_running_loop() | |
| except RuntimeError: | |
| asyncio.run(_wait_for_tasks(tasks_list)) | |
| else: | |
| loop.run_until_complete(_wait_for_tasks(tasks_list)) | |
| asyncio.run(_wait_for_tasks(tasks_list)) |
| background_task_exceptions: deque[Exception] = deque( | ||
| maxlen=100 # don't grow infinitely in production | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making the exception buffer size configurable.
The maxlen=100 limit for background exceptions is hardcoded. For high-throughput applications or debugging scenarios, this might be too small. Consider making it configurable via an environment variable.
Example:
+import os
+
+MAX_EXCEPTIONS = int(os.getenv("MAX_BACKGROUND_EXCEPTIONS", "100"))
+
background_task_exceptions: deque[Exception] = deque(
- maxlen=100 # don't grow infinitely in production
+ maxlen=MAX_EXCEPTIONS # don't grow infinitely in production
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| background_task_exceptions: deque[Exception] = deque( | |
| maxlen=100 # don't grow infinitely in production | |
| ) | |
| import os | |
| MAX_EXCEPTIONS = int(os.getenv("MAX_BACKGROUND_EXCEPTIONS", "100")) | |
| background_task_exceptions: deque[Exception] = deque( | |
| maxlen=MAX_EXCEPTIONS # don't grow infinitely in production | |
| ) |
🤖 Prompt for AI Agents
In template/copier_template_resources/{% if
template_might_want_to_use_python_asyncio %}python_asyncio{% endif
%}/background_tasks.py around lines 9 to 11, the deque maxlen is hardcoded to
100; change it to read an environment variable (e.g.
BACKGROUND_TASK_EXCEPTIONS_MAXLEN) with a sensible default of 100, parse it as
an integer with safe fallback on invalid values, and validate it is a positive
integer before passing it to deque so the buffer size can be configured
per-deployment.
| except ( # pragma: no cover # hard to unit test this, but it'd be good to think of a way to do so | ||
| asyncio.CancelledError | ||
| ): | ||
| _ = _task_creation_tracebacks.pop(task_id, None) | ||
| return | ||
| except Exception as e: # pragma: no cover # hard to unit test this, but it'd be good to think of a way to do so | ||
| creation_tb = _task_creation_tracebacks.pop(task_id, "No traceback available") | ||
| logger.exception(f"Unhandled exception in background task\nTask was created from:\n{creation_tb}") | ||
| background_task_exceptions.append(e) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Testing suggestion for exception handlers.
The pragma: no cover comments on Lines 21 and 26 acknowledge testing challenges. Consider adding a test helper that creates a task designed to fail, registers it, and verifies the exception is captured.
Example test:
async def test_background_task_exception_captured():
async def failing_task():
raise ValueError("test error")
task = asyncio.create_task(failing_task())
register_task(task)
await asyncio.sleep(0.1) # Let task complete
assert len(background_task_exceptions) == 1
assert isinstance(background_task_exceptions[0], ValueError)| CUSTOM_IGNORED_HOSTS: tuple[str, ...] = () | ||
|
|
||
| ALLOWED_HOSTS.extend(CUSTOM_ALLOWED_HOSTS) | ||
| IGNORED_HOSTS.extend(CUSTOM_IGNORED_HOSTS) |
There was a problem hiding this comment.
Guard against accidental string “custom hosts” input (would extend by characters).
If a user edits CUSTOM_IGNORED_HOSTS to a single string (common mistake), IGNORED_HOSTS.extend(CUSTOM_IGNORED_HOSTS) will add characters instead of a hostname. Consider a runtime assertion to fail fast.
CUSTOM_IGNORED_HOSTS: tuple[str, ...] = ()
+assert not isinstance(CUSTOM_IGNORED_HOSTS, str), "CUSTOM_IGNORED_HOSTS must be a tuple[str, ...], not a str"
IGNORED_HOSTS.extend(CUSTOM_IGNORED_HOSTS)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CUSTOM_IGNORED_HOSTS: tuple[str, ...] = () | |
| ALLOWED_HOSTS.extend(CUSTOM_ALLOWED_HOSTS) | |
| IGNORED_HOSTS.extend(CUSTOM_IGNORED_HOSTS) | |
| CUSTOM_IGNORED_HOSTS: tuple[str, ...] = () | |
| assert not isinstance(CUSTOM_IGNORED_HOSTS, str), "CUSTOM_IGNORED_HOSTS must be a tuple[str, ...], not a str" | |
| IGNORED_HOSTS.extend(CUSTOM_IGNORED_HOSTS) |
🤖 Prompt for AI Agents
In template/copier_template_resources/{% if template_might_want_to_use_vcrpy
%}vcrpy_fixtures.py{% endif %} around lines 11 to 13, guard against accidental
assignment of CUSTOM_IGNORED_HOSTS to a single string which would cause
IGNOREDS_HOSTS.extend(...) to add characters; add a runtime assertion or
explicit type check that CUSTOM_IGNORED_HOSTS is a tuple (or at least a non-str
iterable) of strings and raise a clear TypeError/AssertionError if not, then
safely call IGNORED_HOSTS.extend(CUSTOM_IGNORED_HOSTS).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
template/copier_template_resources/{% if template_might_want_to_use_vcrpy %}vcrpy_fixtures.py{% endif %} (2)
43-60: Header filtering is fine; considerpop()to simplify and avoid double membership checks.for header in headers_to_filter: - if header in headers: - del headers[header] - if ( - header.lower() in headers - ): # some headers are lowercased by the server in the response (e.g. Date, Server) - del headers[header.lower()] + headers.pop(header, None) + headers.pop(header.lower(), None) # some servers lowercase response headers
33-42: Hardenvcr.match_onmutation: currentcast()won't prevent assertion failures if it's a list.The vcrpy library accepts both
listandtupleformatch_on, butcast()provides only type-hint information and performs no runtime conversion. Ifvcr.match_onis a list, the assertion will fail. Additionally, the current approach will duplicate "body" if it's already present.def pytest_recording_configure( config: pytest.Config, # noqa: ARG001 # the config argument MUST be present (even when unused) or pytest-recording throws an error vcr: VCR, ): - vcr.match_on = cast(tuple[str, ...], vcr.match_on) # pyright: ignore[reportUnknownMemberType] # I know vcr.match_on is unknown, that's why I'm casting and isinstance-ing it...not sure if there's a different approach pyright prefers - assert isinstance(vcr.match_on, tuple), ( - f"vcr.match_on is not a tuple, it is a {type(vcr.match_on)} with value {vcr.match_on}" - ) - vcr.match_on += ("body",) # body is not included by default, but it seems relevant + match_on = cast(object, vcr.match_on) # pyright: ignore[reportUnknownMemberType] + if isinstance(match_on, list): + match_on = tuple(match_on) + if not isinstance(match_on, tuple): + raise TypeError(f"vcr.match_on must be a tuple/list of str, got {type(match_on)}: {match_on!r}") + if "body" not in match_on: + match_on = match_on + ("body",) + vcr.match_on = match_on # pyright: ignore[reportUnknownMemberType]
♻️ Duplicate comments (1)
template/copier_template_resources/{% if template_might_want_to_use_vcrpy %}vcrpy_fixtures.py{% endif %} (1)
15-17: Guard againstCUSTOM_IGNORED_HOSTSbeing set to a string (would extend by characters).CUSTOM_IGNORED_HOSTS: tuple[str, ...] = () +assert not isinstance(CUSTOM_IGNORED_HOSTS, str), ( + "CUSTOM_IGNORED_HOSTS must be a tuple[str, ...], not a str" +) IGNORED_HOSTS.extend(CUSTOM_IGNORED_HOSTS)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
template/copier_template_resources/{% if template_might_want_to_use_vcrpy %}vcrpy_fixtures.py{% endif %}(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint-matrix (ubuntu-24.04, 3.13.1, --data-file tests/copier_data/data2.yaml)
- GitHub Check: lint-matrix (ubuntu-24.04, 3.12.7, --data-file tests/copier_data/data1.yaml)
🔇 Additional comments (1)
template/copier_template_resources/{% if template_might_want_to_use_vcrpy %}vcrpy_fixtures.py{% endif %} (1)
62-62: Hook assignment looks correct.
| UNREACHABLE_IP_ADDRESS = "192.0.2.1" # RFC 5737 TEST-NET-1 | ||
| IGNORED_HOSTS = [ | ||
| "testserver", # Skip recording any requests to our own server - let them run live | ||
| UNREACHABLE_IP_ADDRESS, # allow this through VCR in order to be able to test network failure handling | ||
| ] | ||
| ALLOWED_HOSTS = [] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add type annotations for the new host lists to avoid accidental non-str entries.
Consider:
-IGNORED_HOSTS = [
+IGNORED_HOSTS: list[str] = [
@@
-ALLOWED_HOSTS = []
+ALLOWED_HOSTS: list[str] = []🤖 Prompt for AI Agents
In template/copier_template_resources/{% if template_might_want_to_use_vcrpy
%}vcrpy_fixtures.py{% endif %} around lines 8–13 the list constants
IGNORE_HOSTS/ALLOWED_HOSTS lack type annotations; add explicit typing (e.g.,
from typing import List and annotate UNREACHABLE_IP_ADDRESS: str = "...",
IGNORED_HOSTS: List[str] = [...], ALLOWED_HOSTS: List[str] = []) so static
checkers and linters will enforce string-only entries.
...ier_template_resources/{% if template_might_want_to_use_vcrpy %}vcrpy_fixtures.py{% endif %}
Show resolved
Hide resolved
1 participant
Why is this change necessary?
Allow/ignore hosts wasn't being used correctly
How does this change address the issue?
ignores things we want to be able to reach, and only uses "allow" for funky windows CI stuff
Adds in standardized fixtures for handling background tasks with asyncio and confirming in the test suite that they're not failing
What side effects does this change have?
N/A
How is this change tested?
Downstream repas
Other
pre-commit CI jobs now display a diff if they fail
Switched to using
-style lists in github CI job config to minimize git diffsSummary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.