Rewrite integration test in pytest #606

martinpitt · 2024-03-18T09:55:23Z

Move it from tasks/ into test/ as it involves more than the tasks container, and could e.g. also grow integration tests for metrics.

Also set up ruff and mypy static code checks, and add a pyproject.toml configuration for these.

Fix mysterious S3 404 (works with local run)
Bring back "sit" behaviour on failures
Use locally built container if it changed in a PR: demo run on commit 027dd89 (with touching container)
File a bug for S3 host port (not a blocker, but inconvenient): reported to PortForwarding(host_port=...) has no effect dcermak/pytest_container#190
Fix for pytest -nauto (with sudo dnf install python3-pytest-xdist)

martinpitt · 2024-03-18T13:55:30Z

@allisonkarlitskaya This is ready for review now -- there are still a few hacks for pytest_container issues, but upstream is very nice and responsive, so over time we can hopefully clean them up.

allisonkarlitskaya

One very quick comment: please add a separate commit to include all of the test dependencies in the tasks container.

allisonkarlitskaya · 2024-03-18T15:44:41Z

When running this in a toolbox I get this pretty quickly:

ERROR test/test_deployment.py::test_podman - ValueError: Selected runtime podman does not exist on the system

Even though I have a (script wrapper) podman in my path.

This would be a fairly serious regression...

martinpitt · 2024-03-18T16:07:00Z

One very quick comment: please add a separate commit to include all of the test dependencies in the tasks container.

I can split it up for the previously existing pyflakes/pycodestyle, but the pytest_container, ruff, and mypy stuff is definitively new and should be in the same commit.

ValueError: Selected runtime podman does not exist on the system

Ah right! That's another quirk of pytest_container which I should report -- sudo dnf install buildah will fix that (but it shouldn't need that as we don't build anything). I reported that here: dcermak/pytest_container#191

martinpitt · 2024-03-19T10:02:57Z

@allisonkarlitskaya I fixed the test to be parallel friendly. I can now run two pytest simultaneously.

martinpitt · 2024-03-20T04:56:47Z

@allisonkarlitskaya I added a separate FIXUP to use pytest_container from main. That makes things much nicer. If you object to that, I can drop it, otherwise squash.

martinpitt · 2024-03-20T08:30:01Z

I rewrote this without pytest_container at last. It avoids dependencies, actually makes things simpler (as we don't use the advanced capabilities of pytest_container like building containers on the fly), avoids a few hacks especially around PULL_ALWAYS, and even reduces the PR by 40 lines.

Initially pushed as separate fixup in commit 2593b1e , just for archival purposes. I want to land this version, so I'll squash it now and adjust the documentation.

martinpitt · 2024-03-20T08:44:35Z

@allisonkarlitskaya I fixed parallel runs -- restoring the podman socket permissions on teardown isn't compatible with parallel tests.

martinpitt · 2024-03-20T08:52:34Z

In installed python3-pytest-xdist and pytest -nauto works now, with some timeout adjustments

martinpitt · 2024-03-20T08:53:31Z

One very quick comment: please add a separate commit to include all of the test dependencies in the tasks container.

Obsolete now -- all of them (pytest, ruff, mypy) are already in tasks.

allisonkarlitskaya

This looks awesome, both compared to the shell version, and the iterations that came before.

For me, the most uncomfortable part about your usage of pytest is how you have a bunch of fixtures which cause side-effects in their environment, but don't return anything. You then use those fixtures from your test functions (while ignoring their values).

I've given a few comments about ways in which I'd change that.

allisonkarlitskaya · 2024-03-21T11:48:48Z

.github/workflows/tests.yml

-          sudo apt-get install -y make python3-pyflakes python3-pycodestyle
+          sudo apt-get install -y make python3-pyflakes python3-pycodestyle python3-pip python3-pytest
+          # `pip install .[test]` does not work properly on Ubuntu 22.04
+          sudo pip install ruff mypy types-PyYAML


I'd almost have preferred an ignore on import yaml, but this doesn't hurt anybody, I guess...

The use of yaml in the test is so trivial. I wonder if we could just avoid it entirely. I guess you want to make a point of testing the deployment, to the extent possible, though...

Right, it's the same code as the actual deployment, and dissecting a k8s resource isn't that trivial with just string functions. I'm ok with ignoring yaml for mypy, but it seemed nice to cover, as that types lib exists.

allisonkarlitskaya · 2024-03-21T11:50:27Z

Makefile

@@ -6,6 +6,8 @@ all:
 check:
 	python3 -m pyflakes tasks tasks/container/webhook
 	python3 -m pycodestyle --max-line-length=120 --ignore=E722 tasks tasks/container/webhook
+	if command -v mypy >/dev/null && pip show types-PyYAML >/dev/null; then mypy test; else echo "SKIP: mypy or types-PyYAML not installed"; fi


This annoys me more. I don't think we should skip mypy because we don't have the yaml types installed...

Yeah, that's something I'd like to do as a follow-up, or prerequisite. It involves a tasks container refresh to add it there, as the integration test runs itself inside of the tasks container, and would fail without it. I sent #609, if/when that lands I can drop the pip show part.

OK, # type: ignore[import-untyped] DTRT, and still typechecks if the types are actually installed. So I went with that.

allisonkarlitskaya · 2024-03-21T11:51:53Z

tasks/README.md


 This will run tests-scan/tests-trigger on the given PR and trigger an
 [unit-tests](../.cockpit-ci/run) test which simply does `make check`.

 You can get an interactive shell with

-    tasks/run-local.sh -i
+    pytest -sm shell


this is seriously l33t.

The addopts="-m 'not shell'" this is a bit less l33t. I guess there's no better way to do that...

If we're going to do that anyway, why do we need the marker? The name of the test would be enough on its own and then you could use -k shell. I don't mind either way, though.

Another option: you add a bunch of commandline arguments via conftest. You could do --shell mode there.

@pytest.mark is the only way that I found how to properly decorate a test to skip itself. You don't have access to the pytestconfig fixture (which is a runtime concept) in a decorator. I suppose I can convert it from a decorator into a procedural call though. But then I'd have to add this check to all other tests (to skip themselves in --shell mode), which I find super ugly.

So in essence, I need an elegant way to say "discover/run these tests by default", but with some special invocation only run test_shell; -k only gives me half of that.

If you have a better idea, I'm all ears! I don't like the "not shell" hack either, but I don't know a better way.

You can implement commandline argument parsing via plugins and you can modify test collection via plugins. I feel like that's all you need?

Ah, thanks for the hint! That goes to the pilot board as improvement then. I'll probably look at it in the next days, I want to learn this.

allisonkarlitskaya · 2024-03-21T11:54:29Z

test/test_deployment.py

+
+    # we want to have useful pod/container names for interactive debugging and log dumping, but still allow
+    # parallel tests (with e.g. xdist), so disambiguate them with the pid
+    test_instance = str(os.getpid())


I love this new approach. Thanks for that.

allisonkarlitskaya · 2024-03-21T11:54:45Z

test/test_deployment.py

+
+
+@pytest.fixture(scope='session')
+def pod(config: Config, pytestconfig) -> Generator[PodData, None, None]:


Better written Iterator[PodData] imho (or was it Iterable?), but it's a matter of taste.

Oh, that's nicer indeed! I was following mypy's recommendation without thinking about it too much. Changed to Iterator.

allisonkarlitskaya · 2024-03-21T11:59:27Z

test/test_deployment.py

+
+    yield f'export GITHUB_API={GHAPI_URL_POD}; SHA={bots_sha}'
+
+    exec_c_out(pod.tasks, f'while kill {mock_pid}; do sleep 0.1; done')


Why can't we just create a subprocess object and use the normal .kill() function on it when we're done?

We can, but then it doesn't fit nicely into the exec_c{,_out}() helpers any more, with subprocess.run(), plus it makes the whole podman exec stick around. But I rewrote it using Popen() now, and it does look quite a bit nicer.

allisonkarlitskaya · 2024-03-21T12:00:26Z

test/test_deployment.py

+def mock_git_push(pod: PodData) -> Generator[None, None, None]:
+    """Install tasks/mock-git-push"""
+
+    subprocess.run(['podman', 'cp', str(ROOT_DIR / 'tasks/mock-git-push'), f'{pod.tasks}:/usr/local/bin/git'],


I wonder if we can just include this in the pod setup? Is there ever a time we want to push "for real"?

The interaction of one fixture implicitly modifying the state of another fixture (and then returning None for itself) seems a bit magic to me.

Right, moved to the setup.

allisonkarlitskaya · 2024-03-21T12:02:12Z

test/test_deployment.py

+    generate_config(config,
+                    forge_opts=f'api-url = "{GHAPI_URL_POD}"',
+                    run_args=f'"--pod={pod.pod}", "--env=GITHUB_API={GHAPI_URL_POD}"')
+    return None  # silence ruff PT004


I'd much rather these returned a path object which we could pass to job-runner -F...

But then we'd diverge from what happens in the actual deployment, where we don't use -F, and instead point to it via env variable. I can return the path of course, but it's not actually going to use it. It looks a bit nicer, though.

This is a prerequisite for proper mypy testing of the upcoming pytest in PR #606

Move it from tasks/ into test/ as it involves more than the tasks container, and could e.g. also grow integration tests for metrics. Also set up ruff and mypy static code checks, and add a pyproject.toml configuration for these.

martinpitt · 2024-03-21T14:56:08Z

@allisonkarlitskaya I addressed most of your comments, I just don't have a better idea about the -sm shell, see the discussion above.

allisonkarlitskaya

I'm happy enough with this now. We can address remaining issues/tweaks as follow-ups :)

This is a very nice improvement. I'm glad you were inspired to work on it ✨

This is a prerequisite for proper mypy testing of the upcoming pytest in PR #606

martinpitt · 2024-03-21T16:13:51Z

Ack! Let's see how this works in practice. I'll work on a corresponding bots change to use the new test there also, otherwise we break PRs. I added the "shell hack" thing to the pilot board.

`run-local.sh` was replaced with pytest. See cockpit-project/cockpituous#606

This is a prerequisite for proper mypy testing of the upcoming pytest in PR #606

`run-local.sh` was replaced with pytest. See cockpit-project/cockpituous#606

martinpitt force-pushed the pytest branch 9 times, most recently from 027dd89 to 8ee38d0 Compare March 18, 2024 13:50

martinpitt marked this pull request as ready for review March 18, 2024 13:51

martinpitt requested a review from allisonkarlitskaya March 18, 2024 13:54

allisonkarlitskaya reviewed Mar 18, 2024

View reviewed changes

martinpitt mentioned this pull request Mar 18, 2024

requires buildah even without container builds, confusing error message dcermak/pytest_container#191

Closed

This comment was marked as outdated.

Sign in to view

martinpitt force-pushed the pytest branch from c44ef52 to f006e6a Compare March 19, 2024 09:54

martinpitt requested a review from allisonkarlitskaya March 19, 2024 10:03

martinpitt force-pushed the pytest branch 3 times, most recently from bef8a31 to d14efe9 Compare March 20, 2024 04:54

martinpitt force-pushed the pytest branch 3 times, most recently from 1c5a389 to 4148c63 Compare March 20, 2024 08:43

martinpitt force-pushed the pytest branch from 4148c63 to 0d0878c Compare March 20, 2024 08:52

allisonkarlitskaya reviewed Mar 21, 2024

View reviewed changes

martinpitt added a commit that referenced this pull request Mar 21, 2024

tasks: Install types-PyYAML into container

41670c8

This is a prerequisite for proper mypy testing of the upcoming pytest in PR #606

martinpitt mentioned this pull request Mar 21, 2024

tasks: Install types-PyYAML into container #609

Merged

Rewrite integration test in pytest

4da9078

Move it from tasks/ into test/ as it involves more than the tasks container, and could e.g. also grow integration tests for metrics. Also set up ruff and mypy static code checks, and add a pyproject.toml configuration for these.

martinpitt force-pushed the pytest branch from 0d0878c to 4da9078 Compare March 21, 2024 14:55

martinpitt requested a review from allisonkarlitskaya March 21, 2024 14:58

allisonkarlitskaya approved these changes Mar 21, 2024

View reviewed changes

martinpitt added a commit that referenced this pull request Mar 21, 2024

tasks: Install types-PyYAML into container

19e77ea

This is a prerequisite for proper mypy testing of the upcoming pytest in PR #606

martinpitt merged commit 32b20aa into main Mar 21, 2024
3 checks passed

martinpitt deleted the pytest branch March 21, 2024 16:13

martinpitt added a commit to cockpit-project/bots that referenced this pull request Mar 21, 2024

workflows: Adjust to cockpituous' new pytest

4f53db8

`run-local.sh` was replaced with pytest. See cockpit-project/cockpituous#606

martinpitt mentioned this pull request Mar 21, 2024

workflows: Adjust to cockpituous' new pytest cockpit-project/bots#6126

Merged

martinpitt added a commit that referenced this pull request Mar 21, 2024

tasks: Install types-PyYAML into container

0a72ff7

This is a prerequisite for proper mypy testing of the upcoming pytest in PR #606

martinpitt added a commit that referenced this pull request Mar 21, 2024

tasks: Install types-PyYAML into container

fa620ea

This is a prerequisite for proper mypy testing of the upcoming pytest in PR #606

allisonkarlitskaya pushed a commit to cockpit-project/bots that referenced this pull request Mar 21, 2024

workflows: Adjust to cockpituous' new pytest

5ab1de5

`run-local.sh` was replaced with pytest. See cockpit-project/cockpituous#606



		@pytest.fixture(scope='session')
		def pod(config: Config, pytestconfig) -> Generator[PodData, None, None]:


		yield f'export GITHUB_API={GHAPI_URL_POD}; SHA={bots_sha}'

		exec_c_out(pod.tasks, f'while kill {mock_pid}; do sleep 0.1; done')

Rewrite integration test in pytest #606

Rewrite integration test in pytest #606

Conversation

martinpitt commented Mar 18, 2024 • edited Loading

martinpitt commented Mar 18, 2024

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

allisonkarlitskaya commented Mar 18, 2024

martinpitt commented Mar 18, 2024 • edited Loading

This comment was marked as outdated.

martinpitt commented Mar 19, 2024

martinpitt commented Mar 20, 2024

martinpitt commented Mar 20, 2024

martinpitt commented Mar 20, 2024

martinpitt commented Mar 20, 2024

martinpitt commented Mar 20, 2024

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Mar 21, 2024

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

martinpitt commented Mar 21, 2024

martinpitt commented Mar 18, 2024 •

edited

Loading

martinpitt commented Mar 18, 2024 •

edited

Loading