Create new markers for requiring in process execution or docker #13003
Conversation
Test Results - Preflight, Unit22 107 tests 20 372 ✅ 6m 56s ⏱️ Results for commit ff52d68. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Acceptance7 tests 5 ✅ 3m 12s ⏱️ Results for commit ff52d68. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 20m 28s ⏱️ Results for commit ff52d68. ♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files 2 suites 1h 42m 9s ⏱️ Results for commit ff52d68. ♻️ This comment has been updated with latest results. |
e4dcd34 to
1f0d400
Compare
| @@ -62,7 +62,12 @@ class Markers: | |||
| only_on_amd64 = pytest.mark.only_on_amd64 | |||
| only_on_arm64 = pytest.mark.only_on_arm64 | |||
| resource_heavy = pytest.mark.resource_heavy | |||
| only_in_docker = pytest.mark.only_in_docker | |||
| # Tests require LocalStack to be run inside a container | |||
| requires_in_container = pytest.mark.requires_in_container | |||
There was a problem hiding this comment.
suggestion: is there a way to add a docstring for these? So we can hover over the definition to get this comment?
| requires_in_container = pytest.mark.requires_in_container | |
| requires_in_container = pytest.mark.requires_in_container | |
| """Tests require LocalStack to be run inside a container""" |
This would be good to add to the other markers as well
There was a problem hiding this comment.
Good point, will add them!
There was a problem hiding this comment.
Added them, and the IDE shows them as expected:
| if not is_in_docker and "only_in_docker" in item.keywords: | ||
| item.add_marker(only_in_docker) | ||
| if not is_in_docker and "requires_in_container" in item.keywords: | ||
| item.add_marker(requires_in_container) | ||
| if is_in_ci and not is_amd64 and "only_on_amd64" in item.keywords: | ||
| item.add_marker(only_on_amd64) | ||
| if is_in_ci and not is_arm64 and "only_on_arm64" in item.keywords: |
There was a problem hiding this comment.
suggestion: could we do something like:
requires_in_process = pytest.mark.skip(
reason="..."
)
# ...
if localstack_config.is_env_true("TEST_SKIP_LOCALSTACK_START") and "requires_in_process" in item.keywords:
item.add_marker(requires_in_process)?
There was a problem hiding this comment.
Yes we can, but I am not entirely confident we should, it might get confusing to have these skips. In the k8s pipeline we manually skip them for now, and there was no other usecase until now. I'm happy with adding it, but I think this can wait until we actually encounter a problem this will solve, WDYT?
There was a problem hiding this comment.
Makes sense. You are right, we don't want to (silently) skip these specific tests without warning the user and the CI will opt in. If we execute tests against a running container and look at the failures, we will clearly see the new marker which will explain why the test fails easily. I'm ok with this 👍
There was a problem hiding this comment.
I think a few tests are missing, from my recent investigation into parallelising the CFn tests, I found these three tests require in-process access:
- tests/aws/services/cloudformation/api/test_stacks.py::TestStacksApi::test_create_stack_with_custom_id (requires in memory access to the provider, however it's not super clear. It uses the
set_resource_custom_idfixture) - tests/aws/services/cloudformation/resources/test_apigateway.py::test_cfn_apigateway_swagger_import (uses
echo_http_server_post) - tests/aws/services/cloudformation/resources/test_lambda.py::test_lambda_cfn_run_with_non_empty_string_replacement_deny_list (uses
monkeypatch)
Have we verified these markers in this community PR?
|
They have been verified as part of our |
Motivation
We currently have no way of marking tests as requiring docker or requiring to run in the same process as the LS instance (usually for monkeypatching / pytest http server usage).
This makes certain test scenarios harder, for example testing against a running LS instance, or testing against kubernetes.
With this PR, new markers are introduced:
requires_docker: This marker should be set if a test requires the docker daemon (or a compatible implementation, like podman with docker socket support) to run.requires_in_process: This marker should be set if a test needs to run inside the same process (or network namespace) as LocalStack. Usually used for tests withmonkeypatchorpytest-httpserver, with the latter hopefully fixed in the future.requires_in_container: For better distinction between this marker andrequires_dockerthis is a renamedonly_in_docker- it requires LocalStack to run in a container (usually for system level dependency installation), no matter if it is a docker container or kubernetes pod.Changes
only_in_dockertorequires_in_containerrequires_dockerandrequires_in_process