feat: Custom container factory support - #1
Conversation
Mesa DescriptionThis PR introduces a major feature enhancement: the ability to use a custom factory function to create and manage test containers. This provides significantly more flexibility for complex test setups compared to the existing methods of specifying a Docker image or build path. The core of the change is the refactoring of the To support this, the internal type system was updated with a new Additionally, the container bootstrapping process has been significantly improved. It now dynamically locates a Python executable within the container and creates a dedicated virtual environment for installing dependencies. This makes the test execution environment more robust and reliable, which is especially important when dealing with a wide variety of custom user-provided containers. Finally, the changes are well-documented in the README and thoroughly tested with a new suite of end-to-end tests verifying the factory functionality. Description generated by Mesa. Update settings |
Unify build_container_spec_from_args with the decorator's _build_spec into a single function with explicit named parameters and overloads. Add FactorySpec handling to the plugin's _run_test_in_container.
There was a problem hiding this comment.
Performed full review of 8e6da17...931b882
Analysis
-
The PR extends the public API surface without ensuring backward compatibility for positional arguments in the
@in_container(path, tag)and@pytest.mark.in_container(path, tag)decorators. -
There are no automated regression tests covering the legacy decorator forms, which creates risk as the type system evolves.
-
The introduction of a third ContainerSpec branch (FactorySpec) increases complexity in the codebase that may need additional maintenance overhead.
-
The bootstrap refactor in
_container.pymakes execution more resilient, but could introduce new edge cases or failure modes in diverse container environments that should be carefully verified.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
9 files reviewed | 2 comments | Edit Agent Settings • Read Docs
| def in_container(*, factory: ContainerFactory) -> Callable[[Callable[P, T]], Callable[P, T]]: ... | ||
|
|
||
|
|
||
| def in_container( |
There was a problem hiding this comment.
The updated signature only permits a single positional argument (image) and makes path/tag keyword-only. The previous release explicitly supported @in_container("./docker", "my-tag"), so every existing decorator that passes path/tag positionally will now crash with TypeError: in_container() takes 1 positional argument but 2 were given before we even reach build_container_spec_from_args. That is a breaking API change for the public decorator. Could we keep accepting the historical (path, tag) positional form (e.g. by restoring *args handling or by detecting two positional strings before binding to keywords) so downstream tests don’t break when upgrading?
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: mesa-dot-dev/pytest-in-docker#1
File: pytest_in_docker/_decorator.py#L50
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The updated signature only permits a single positional argument (`image`) and makes `path`/`tag` keyword-only. The previous release explicitly supported `@in_container("./docker", "my-tag")`, so every existing decorator that passes `path`/`tag` positionally will now crash with `TypeError: in_container() takes 1 positional argument but 2 were given` before we even reach `build_container_spec_from_args`. That is a breaking API change for the public decorator. Could we keep accepting the historical `(path, tag)` positional form (e.g. by restoring `*args` handling or by detecting two positional strings before binding to keywords) so downstream tests don’t break when upgrading?
|
|
||
| def build_container_spec_from_args(*args: str, **kwargs: str) -> ContainerSpec: | ||
| def build_container_spec_from_args( | ||
| image: str | None = None, |
There was a problem hiding this comment.
build_container_spec_from_args used to accept (path, tag) or even (path,), {"tag": ...} via the *args/**kwargs matcher. The new keyword-only signature rejects those forms, so code like @pytest.mark.in_container("./docker", "my-tag") (or the same coming through marker.args) now raises TypeError before we can even validate the spec. Even the mixed form @pytest.mark.in_container("./docker", tag="my-tag") silently treats the positional string as an image and ignores the explicit tag, changing behavior without an error. Could we keep the previous flexible parsing (or normalize the args inside the plugin) so existing marks still work and the positional (path, tag) form doesn’t regress?
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: mesa-dot-dev/pytest-in-docker#1
File: pytest_in_docker/_types.py#L75
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
`build_container_spec_from_args` used to accept `(path, tag)` or even `(path,), {"tag": ...}` via the `*args/**kwargs` matcher. The new keyword-only signature rejects those forms, so code like `@pytest.mark.in_container("./docker", "my-tag")` (or the same coming through `marker.args`) now raises `TypeError` before we can even validate the spec. Even the mixed form `@pytest.mark.in_container("./docker", tag="my-tag")` silently treats the positional string as an image and ignores the explicit tag, changing behavior without an error. Could we keep the previous flexible parsing (or normalize the args inside the plugin) so existing marks still work and the positional `(path, tag)` form doesn’t regress?
No description provided.