Skip to content

[autobackport: sssd-2-10] Tests: Add integration tests validating SSSD socket#8562

Open
sssd-bot wants to merge 1 commit intoSSSD:sssd-2-10from
sssd-bot:SSSD-sssd-backport-pr8481-to-sssd-2-10
Open

[autobackport: sssd-2-10] Tests: Add integration tests validating SSSD socket#8562
sssd-bot wants to merge 1 commit intoSSSD:sssd-2-10from
sssd-bot:SSSD-sssd-backport-pr8481-to-sssd-2-10

Conversation

@sssd-bot
Copy link
Copy Markdown
Contributor

This is an automatic backport of PR#8481 Tests: Add integration tests validating SSSD socket to branch sssd-2-10, created by @aborah-sudo.

Please make sure this backport is correct.

Note

The commits were cherry-picked without conflicts.

You can push changes to this pull request

git remote add sssd-bot git@github.com:sssd-bot/sssd.git
git fetch sssd-bot refs/heads/SSSD-sssd-backport-pr8481-to-sssd-2-10
git checkout SSSD-sssd-backport-pr8481-to-sssd-2-10
git push sssd-bot SSSD-sssd-backport-pr8481-to-sssd-2-10 --force

Original commits
abee6e7 - Tests: Add integration tests validating SSSD socket

Backported commits

  • 1f3a7ab - Tests: Add integration tests validating SSSD socket

Original Pull Request Body

Add integration tests validating SSSD socket activation behavior for individual responders and mixed socket/traditional configurations.

Add integration tests validating SSSD socket activation behavior
for individual responders and mixed socket/traditional configurations.

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
Reviewed-by: Jakub Vávra <jvavra@redhat.com>
(cherry picked from commit abee6e7)
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new suite of system tests for SSSD socket activation, covering responder lifecycles, autofs support, mixed-mode configurations, and configuration conflicts. The review feedback correctly identifies that while the test docstrings claim to verify that services are initially inactive, the actual implementation lacks these assertions. Actionable suggestions were provided to add checks ensuring service units are not running before the first client request is made.

socket_unit = f"sssd-{responder}.socket"
service_unit = f"sssd-{responder}.service"

assert client.sssd.svc.is_active(socket_unit), f"{responder} socket should be active"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The test's docstring states that it verifies that the service unit is inactive initially. However, this crucial assertion is missing. To properly test the socket activation lifecycle, please add a check to confirm the service is not running before the first client request is made.

Suggested change
assert client.sssd.svc.is_active(socket_unit), f"{responder} socket should be active"
assert client.sssd.svc.is_active(socket_unit), f"{responder} socket should be active"
assert not client.sssd.svc.is_active(service_unit), f"{responder} service should be inactive initially"

socket_unit = f"sssd-{responder}.socket"
service_unit = f"sssd-{responder}.service"

assert client.sssd.svc.is_active(socket_unit), f"{responder} socket should be active"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the other lifecycle tests, this one is missing an assertion to verify that the autofs service unit is inactive before the first request. The test's docstring indicates this check should be performed. Please add it to ensure the test correctly validates the socket activation behavior.

Suggested change
assert client.sssd.svc.is_active(socket_unit), f"{responder} socket should be active"
assert client.sssd.svc.is_active(socket_unit), f"{responder} socket should be active"
assert not client.sssd.svc.is_active(service_unit), f"{responder} service should be inactive initially"

socket_unit = f"sssd-{socket_responder}.socket"
socket_service = f"sssd-{socket_responder}.service"

assert client.sssd.svc.is_active(socket_unit), f"{socket_responder} socket should be active"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This test for mixed-mode configuration is missing a key assertion. The docstring mentions verifying that the socket-activated responder is initially inactive, but the code doesn't check this. Please add an assertion to verify the service is not active before the request is triggered.

Suggested change
assert client.sssd.svc.is_active(socket_unit), f"{socket_responder} socket should be active"
assert client.sssd.svc.is_active(socket_unit), f"{socket_responder} socket should be active"
assert not client.sssd.svc.is_active(socket_service), f"{socket_responder} service should be inactive initially"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants