test: Add Database Tests For Container Images - Postgres, Redis, Valkey, Memcached - #18001
test: Add Database Tests For Container Images - Postgres, Redis, Valkey, Memcached#18001MadhurAggarwal wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds container runtime coverage for four datastore packages.
Changes:
- Adds version and core functionality tests for Valkey, Redis compatibility, PostgreSQL, and Memcached.
- Adds Dockerfiles installing each datastore package.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
test_valkey/test_valkey.py |
Tests Valkey startup, version, and list operations. |
test_valkey/Dockerfile |
Installs Valkey. |
test_redis/test_redis.py |
Tests Redis-compatible commands and list operations. |
test_redis/Dockerfile |
Installs the Redis compatibility package. |
test_postgres/test_postgres.py |
Tests PostgreSQL initialization, connectivity, and SQL operations. |
test_postgres/Dockerfile |
Installs PostgreSQL client and server. |
test_memcached/test_memcached.py |
Tests Memcached startup and set/get behavior. |
test_memcached/Dockerfile |
Installs Memcached and Ncat. |
|
|
||
|
|
||
| @pytest.mark.dockerfile() | ||
| def test_redis_lpush(container_exec_shell) -> None: |
There was a problem hiding this comment.
Current 4.0 DB ports validate redis/valkey/postgres/memcached via localhost inside one container.
3.0 golden tests included network-path coverage (separate client/server containers or host->container path).
@christopherco Do we want to keep 4.0 scope as package/runtime smoke only, or should we add at least one cross-container connectivity test per DB for parity
There was a problem hiding this comment.
I've created a fixture client_server_exec_shell and used it for cross-container tests in Postgres, Redis and Valkey.
I have not yet added any cross-container tests for memcached since the 3.0 implementation doesn't have it either. Should I add that as well?
9add5b3 to
5976ce1
Compare
| return resolve_image_reference(podman_client, image_path=image_path, image_ref=image_ref) | ||
|
|
||
|
|
||
| def _effective_image(podman_client, container_image_ref: str, request: pytest.FixtureRequest) -> str: |
| network_name = f"azl-test-net-{uuid.uuid4().hex[:12]}" | ||
| podman_client.network.create(network_name) | ||
|
|
||
| def _exec_shell_for(container): |
|
I have resolved the previous comments and made the following changes:
I've tested these changes and updated the PR body with latest test run logs images. |
91aeae6 to
ecd40ba
Compare
|
(rebased & resolved merge conflicts and copilot comments) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
base/images/tests/conftest.py:312
- Unlike pytest-injected test parameters, this is an ordinary helper parameter.
podman_clienthas no type, so Pyright reportsreportMissingParameterTypehere underpyrightconfig.json:16; annotate it asDockerClientand import that type.
def _effective_image(podman_client, container_image_ref: str, request: pytest.FixtureRequest) -> str:
base/images/tests/conftest.py:449
- This nested helper is not fixture-injected, and its untyped
containerparameter causes Pyright'sreportMissingParameterTypeplus an unknowncontainer_namemember access. Annotate it withContainerInstanceand give both nested callables concreteContainerExecResult/Callablereturn types.
def _exec_shell_for(container):
def _exec_shell(command: str, *, shell: str = "bash"):
return exec_in_container(podman_client, container.container_name, [shell, "-c", command])
return _exec_shell
tobiasb-ms
left a comment
There was a problem hiding this comment.
issue(blocking): In addition to my code comments, most of the commits in this PR don't conform to conventional commits, which is required. Most likely you want to do a squash rebase anyway, but either way fix that please.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
base/images/tests/conftest.py:312
- This new non-fixture helper leaves
podman_clientuntyped, which adds areportMissingParameterType/reportUnknownParameterTypeerror underpyrightconfig.json:16-18. The Python guidance requires touched files not to introduce new Pyright violations (.github/instructions/python.instructions.md:12-18,29); annotate this parameter asDockerClient.
def _effective_image(podman_client, container_image_ref: str, request: pytest.FixtureRequest) -> str:
base/images/tests/conftest.py:449
containeris an untyped parameter on this newly added helper, so Pyright reports missing/unknown parameter types here (pyrightconfig.json:16-18). Please annotate it asContainerInstanceand give the helper a concrete callable return type; this also makes the yielded fixture contract checkable.
def _exec_shell_for(container):
def _exec_shell(command: str, *, shell: str = "bash"):
return exec_in_container(podman_client, container.container_name, [shell, "-c", command])
return _exec_shell
5a7ec6d to
921ceea
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
base/images/tests/conftest.py:312
_effective_imageis a regular helper (not a pytest-injected test function), but its newpodman_clientparameter has no concrete type. This produces areportMissingParameterTypeerror underpyrightconfig.json:16, contrary to the touched-file requirement in.github/instructions/python.instructions.md:12-18,29. Import and useDockerClienthere so this refactor remains type-checkable.
def _effective_image(podman_client, container_image_ref: str, request: pytest.FixtureRequest) -> str:
|
Squashed into one commit
|
| for attempt in range(attempts): | ||
| if attempt: | ||
| time.sleep(delay) | ||
| result = exec_shell(command) |
tobiasb-ms
left a comment
There was a problem hiding this comment.
Thanks for updating everything. There's still one blocking thing: #18001 (comment), and I left a non-blocking question as well.
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def _postgres_password_file(container_exec_shell, password: str): |
There was a problem hiding this comment.
question(non-blocking): Do you think this could be more generically useful? What this function does is:
- Create a temp file name on the container (completely generic)
- Put a string into that file (already parameterized)
- Change the owner of that file (could easily be parameterized)
- Removes that file as the owner (could be parameterized along with 3)
If you parameterized 3 (and therefor 4), then this function is simply something like container_temp_file and any test can use it.
I'm totally fine punting that though -- we don't actually know we'll need it for anything else -- but keep it in mind if you see opportunities for it in the future.
327f37e to
754e392
Compare
754e392 to
b7ab464
Compare
There was a problem hiding this comment.
🟢 Ready to approve
The datastore tests and shared fixture changes consistently cover the stated local and cross-container scenarios.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Commit 1 - Added Container Runtime Tests for 4 DataStores:
Also Added a new fixture:
client_server_shell_exec:clientandserverrespectivelyEach added test contains:
PR Testing:
Locally built a container images and tested using azldev tool:
Commit 2 - Fixed Pyright Errors:
reportUnknownParameterType,reportMissingParameterType)reportArgumentType,reportReturnType,reportInvalidTypeForm,reportCallIssue)reportAttributeAccessIssue)Previous Pyright Errors (Grouped):

Fixed All Pyright Errors in latest commit:
