Make infratest optional #231

lqc · 2024-10-18T09:07:45Z

What problem does this solve

My team is using pytest-containers for some time now, but we faced a lot of problems trying to get it working on Windows. This is mainly due to pytest-testinfra not really supporting that platform.

The change aims to make the core functionalities of the project usable on Windows.

Summary of the changes

Changed pytest-testinfra to an optional dependency installable via [testinfra] extra
The test suite now runs on OS X. It didn't because host.package() is not implement on OS X
The test suite no longer uses host to perform local operations. Instead I implemented the minimal set of functions need by the suite in a new remote attribute.
Added ifaddr library as a test dependency for queries available network interfaces. This is not used outside of tests.
refactored get_selected_platform() and the runtime classes to not run any shell commands while being imported. The absolute path to the podman/docker command is now resolved lazily and the available platform is selected (following the same podman > docker preference).

This removes the use of testinfra from the main codebase and the test suite making it an optional dependency. You can still use "connection.connection" to get testinfra's connection object if you have it installed.

codecov · 2024-10-18T09:31:54Z

Codecov Report

Attention: Patch coverage is 84.53039% with 28 lines in your changes missing coverage. Please review.

Project coverage is 94.31%. Comparing base (1e19e12) to head (35dd906).

Files with missing lines	Patch %	Lines
pytest_container/runtime.py	82.22%	8 Missing and 8 partials ⚠️
pytest_container/container.py	86.48%	8 Missing and 2 partials ⚠️
pytest_container/build.py	80.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
- Coverage   94.88%   94.31%   -0.57%     
==========================================
  Files           9       10       +1     
  Lines        1114     1214     +100     
  Branches      164      182      +18     
==========================================
+ Hits         1057     1145      +88     
- Misses         42       45       +3     
- Partials       15       24       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dcermak

Thank you for this giant PR! I haven't managed to go through this fully and will need a bit of time. In principle I'm not opposed to making testinfra optional, but it must remain usable and preferably via the same API as before, as we heavily rely on testinfra in tests utilizing pytest_container.

Also, we shouldn't decrease test coverage and will need to adjust the docs (but let's figure out the code first)

dcermak · 2024-10-19T08:10:25Z

pyproject.toml

 Sphinx = ">=5.0"
 pytest-rerunfailures = ">=10.2"
 typeguard = ">=2.13"
+ifaddr = "^0.2.0"


Please make this an

Suggested change

ifaddr = "^0.2.0"

ifaddr = ">=0.2.0"

dcermak · 2024-10-19T11:58:02Z

pytest_container/runtime.py

        return self.__class__.__name__


-LOCALHOST = testinfra.host.get_host("local://")


This is a public object, please wrap this in a try: import testinfra … except block

right, missed this.

dcermak · 2024-10-19T12:05:55Z

pytest_container/runtime.py

        )

-    def _get_container_inspect(self, container_id: str) -> Any:
+    def _run_inspect(self, container_id: str) -> Union[Dict[str, Any], None]:


Technically the return type should be Optional[Dict[str, Any]], but I really don't like the option of us returning None here. We should either return an inspect or error out. Otherwise we just push error handling down on the API user.

If I throw an error on missing, I need to catch it in prepare_container (and then check if the error is because missing or parse error) or make a second function for the same thing (like it is now). Didn't want to refactor prepare_container() too much.

dcermak · 2024-10-19T12:06:36Z

pytest_container/runtime.py

-        if len(inspect) != 1:
+        if not isinstance(inspect, list):
+            raise RuntimeError(
+                f"Expected a list of results, but got {inspect} instead"


Did you want to include the type in the error message instead?

Suggested change

f"Expected a list of results, but got {inspect} instead"

f"Expected a list of results, but got {type(inspect)} instead"

No, I wanted to result of the command, so it's clear what went wrong.

dcermak · 2024-10-19T12:14:23Z

pytest_container/runtime.py

+        raise ValueError(f"Invalid CONTAINER_RUNTIME value: {requested}")
+    current = requested
+    if current == "auto":
+        podmain_path = shutil.which("podman")


typo: podmain -> podman

dcermak · 2024-10-19T12:16:32Z

pytest_container/runtime.py

+        resolved_buildah_binary: Optional[str] = None
+
+        if buildah_binary is _AutoDetect.INSTANCE:
+            # try to guess if argument is omitted
+            buildah_binary = shutil.which("buildah")
+
+        if buildah_binary is None:
+            # if explicitly set to None or not found, we don't have buildah
+            resolved_buildah_binary = None
+            self._buildah_functional = False
+
+        elif isinstance(buildah_binary, str):
+            # make sure it's fully resolved and not just a binary name
+            buildah_binary = shutil.which("buildah")
+            self._buildah_functional = (
+                subprocess.run(
+                    [buildah_binary, "--version"], check=False
+                ).returncode
+                == 0
+                if buildah_binary is not None
+                else False
+            )
+            resolved_buildah_binary = buildah_binary


Which high level problem are you trying to solve here?

As with rest, I don't want the checks to run on module imports and I tried to preserve the original logic of detection if someone creates PodmanRuntime() directly. I can simplify this if you don't really care about the detection in that case and only through get_selected_platform()

The detection can be as dead-simple as possible, there's really no need to try be "smart" here as no one requires it (for now).

dcermak · 2024-10-19T12:22:08Z

pytest_container/container.py

+@dataclass(frozen=True)
+class ContainerRemoteEndpoint:
+    _container_id: str
+    _runtime: OciRuntimeBase
+
+    def __post_init__(self) -> None:
+        assert self._container_id, "Container ID must not be empty"
+
+    def check_output(self, cmd: str, strip: bool = True) -> str:
+        """Run a command in the container and return its output."""
+        return self._runtime.run_command(
+            "exec",
+            self._container_id,
+            "/bin/sh",
+            "-c",
+            cmd,
+            strip=strip,
+        )
+
+    def exists(self, command: str) -> bool:
+        """Check if a command exists in the container."""
+        return (
+            self.check_output(f"command -v {command} || echo '<missing>'")
+            != "<missing>"
+        )
+
+    def file(self, path: str) -> "ContainerConnectionFile":
+        return ContainerConnectionFile(path=path, _remote=self)
+
+    def copy(
+        self, local: Union[str, Path], remote: str
+    ) -> "ContainerConnectionFile":
+        """Copy a file from the host to the container."""
+        if isinstance(local, Path):
+            local = str(local.absolute())
+        self._runtime.run_command(
+            "cp", local, f"{self._container_id}:{remote}"
+        )
+        return self.file(remote)
+
+
+@dataclass(frozen=True)
+class ContainerConnectionFile:
+    path: str
+    _remote: ContainerRemoteEndpoint
+
+    def _test(self, test: str) -> bool:
+        return (
+            self._remote.check_output(
+                f"test {test} {self.path} && echo 1 || echo 0"
+            )
+            == "1"
+        )
+
+    @property
+    def exists(self) -> bool:
+        return self._test("-e")
+
+    @property
+    def is_file(self) -> bool:
+        return self._test("-f")
+
+    @property
+    def is_directory(self) -> bool:
+        return self._test("-d")
+
+    @property
+    def content_string(self) -> str:
+        try:
+            return self._remote.check_output(f"cat {self.path}", strip=False)
+        except subprocess.CalledProcessError as e:
+            if "Is a directory" in e.stderr:
+                raise ValueError(f"{self.path} is a directory") from e
+            raise
+
+    def listdir(self) -> List[str]:


I don't think that it is maintainable to start implementing a subset of testinfra in pytest_container. I think all that we should offer is check_output. If a user doesn't wish to use testinfra, then that's fine by me, but then they should handle these checks by themself.

This is only the set that is needed in the test suite. I can change it to private API. I personally, don't have a use for any of it.

dcermak · 2024-10-19T12:26:41Z

pytest_container/runtime.py

+        try:
+            # universal_newlines is an alias for text before python 3.7
+            result = subprocess.check_output(
+                cmd, stderr=subprocess.PIPE, universal_newlines=True
+            )
+        except subprocess.CalledProcessError as exc:
+            if not ignore_errors:
+                _logger.error(exc.stderr)
+                raise exc
+            result = exc.stdout


Why not use subprocess.run() instead?
Also, I would, highly, highly suggest to log the call. Testinfra does this and it is a savior knowing which commands are run.

I'll change it to {{run()}} and add logging. Note that the current code doesn't log any of these either.

The current code does in indeed not log the call, but since this now becomes the replacement of testinfra's check_output, which logs the output & result, I think it should log too.

dcermak · 2024-10-19T12:30:52Z

pyproject.toml

 Sphinx = ">=5.0"
 pytest-rerunfailures = ">=10.2"
 typeguard = ">=2.13"
+ifaddr = "^0.2.0"


To be frankly honest, we do not need ifaddr. Our own testsuite can rely on testinfra, even if testinfra became optional.

Well, then I can't run the test suite on Windows or OS X.

Then please make this a windows & OSX only dependency

dcermak · 2024-10-19T12:39:32Z

tests/test_pod.py

+    with pytest.raises(
+        RuntimeError, match=f"Container with id {name} not found"
+    ):
+        container_runtime.inspect_container(name)


This will inspect the container i_will_fail_to_launch not the pod i_will_fail_to_launch. As we never created that container, this will fail always

Ah, missed the "pod" argument. Will fix this.

dcermak

I've taken a first cursory pass, but I can already tell you that this PR is too large for my taste. You're trying to change too many things in one pull request/commit:

stylistic import changes and reformats (some of them I agree with, others are really a matter of personal preference)
change how buildah is found
container runtime detection in general
introduce the new testinfra compat classes

Therefore, please split this pull request into multiple smaller ones. I would suggest to start with the buildah & autodetection refactoring and then the testinfra compat class.

Also, you entered into the changelog, that the test suite now runs on OSX, but there's no CI run added to that. And I would really like to keep most of the test relying on testinfra, as that is the major focus of pytest_container. I am not opposed to adding new tests though, I'd actually very much prefer that

lqc · 2024-10-20T10:46:58Z

"stylistic import changes and reformats"
I don't care about that, if anything got changed it might be because I used ruff on a file before switching to black. So if you want something changed, let me know

change how buildah is found and container runtime detection in general
Ok, I think I'll start with that.

"that the test suite now runs on OSX, but there's no CI run added to that."
I don't know if the GitHub Max runners have docker available. It's only the "testinfra" package that causes problems. But I'll try to add it. I know for a fact that the Windows runners can't run Linux containers, so won't be able to add that.

I would really like to keep most of the test relying on testinfra, as that is the major focus of pytest_container.
This I do not understand. If they rely on testinfra, I can't run the tests on platforms testinfra doesn't work on (so effectively only some Linux distros). But this library is very useful without testinfra, just to manage containers as fixture in pytests.

lqc added 2 commits October 18, 2024 09:42

feat: make pytest-testinfra optional

e3afa55

This removes the use of testinfra from the main codebase and the test suite making it an optional dependency. You can still use "connection.connection" to get testinfra's connection object if you have it installed.

updated changelog

35dd906

dcermak reviewed Oct 19, 2024

View reviewed changes

dcermak requested changes Oct 19, 2024

View reviewed changes

rcmadhankumar mentioned this pull request Mar 27, 2025

Make test infra optional #239

Open

		return self.__class__.__name__


		LOCALHOST = testinfra.host.get_host("local://")

	f"Expected a list of results, but got {inspect} instead"
	f"Expected a list of results, but got {type(inspect)} instead"

Make infratest optional #231

Are you sure you want to change the base?

Make infratest optional #231

Uh oh!

Conversation

lqc commented Oct 18, 2024

Uh oh!

codecov bot commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dcermak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcermak left a comment

Choose a reason for hiding this comment

Uh oh!

lqc commented Oct 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Oct 18, 2024 •

edited

Loading