Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring of the memcache tests, aiming to reduce code duplication by introducing helper functions and parametrizing tests. While this is a great improvement for maintainability, the new implementation introduces several critical and high-severity issues, including a syntax error, flawed test logic that would cause failures, and several inconsistencies between test names, docstrings, and their implementations. I've detailed these issues in the review comments.
| if order == "after": | ||
| client.sssd.stop() | ||
| if cache == "users": | ||
| client.sssctl.cache_expire(users=True, groups=False) | ||
| elif cache == "groups" or cache == "initgroups": | ||
| client.sssctl.cache_expire(users=False, groups=True) | ||
| else: | ||
| client.sssctl.cache_expire(everything=True) |
There was a problem hiding this comment.
The sssctl command requires the sssd daemon to be running to receive commands. Calling cache_expire after stopping sssd (when order == "after") will cause the test to fail. This test logic is flawed and needs to be revised. Cache invalidation should happen before stopping the daemon.
|
|
||
|
|
||
| @pytest.mark.importance("high") | ||
| pytest.mark.importance("critical") |
There was a problem hiding this comment.
This pytest marker is missing the leading @, which will cause a SyntaxError and prevent the tests from running.
| pytest.mark.importance("critical") | |
| @pytest.mark.importance("critical") |
| 5. Groups have correct names | ||
| :customerscenario: False | ||
| def assert_objects( | ||
| client: Client, objects: dict[str, list[GenericUser | list[GenericGroup]]], cache: str, id: bool = False |
There was a problem hiding this comment.
The type hint for the objects parameter is incorrect. list[GenericUser | list[GenericGroup]] incorrectly suggests that a list could contain a mix of GenericUser objects and list[GenericGroup] objects. The add_objects function returns a dictionary where values are either list[GenericUser] or list[GenericGroup]. The correct type hint for the dictionary's value is list[GenericUser] | list[GenericGroup]. This same correction should be applied to the assert_objects_not_found and assert_group_membership helper functions.
| client: Client, objects: dict[str, list[GenericUser | list[GenericGroup]]], cache: str, id: bool = False | |
| client: Client, objects: dict[str, list[GenericUser] | list[GenericGroup]], cache: str, id: bool = False |
| for initgroup in objects.get("users", []): | ||
| _group = objects.get("groups", [])[-1].name | ||
| result_getent = client.tools.getent.initgroups(initgroup.name) | ||
| assert not result_getent.memberof(_group), f"User '{initgroup.name}' was found in initgroups!" |
There was a problem hiding this comment.
The check for initgroups in this function is flawed. It will raise an IndexError if no groups are present, and it only checks for non-membership of a single, specific group. When the initgroups cache is disabled, the expectation is that the initgroups call should return no supplementary groups at all. The check should be made more robust to assert that the .groups attribute of the result is empty, which was the approach in the previous tests.
| for initgroup in objects.get("users", []): | |
| _group = objects.get("groups", [])[-1].name | |
| result_getent = client.tools.getent.initgroups(initgroup.name) | |
| assert not result_getent.memberof(_group), f"User '{initgroup.name}' was found in initgroups!" | |
| for initgroup in objects.get("users", []): | |
| result_getent = client.tools.getent.initgroups(initgroup.name) | |
| assert not result_getent.groups, f"User '{initgroup.name}' was found in initgroups with non-empty groups!" |
| @pytest.mark.topology(KnownTopologyGroup.AnyProvider) | ||
| @pytest.mark.preferred_topology(KnownTopology.LDAP) | ||
| def test_memcache__group_cache_disabled_and_lookup_groups(client: Client, provider: GenericProvider): | ||
| def test_memcache__lookup_objects_by_name_with_fully_qualified_names(client: Client, provider: GenericProvider): |
There was a problem hiding this comment.
The test name test_memcache__lookup_objects_by_name_with_fully_qualified_names suggests it covers all "objects" (which includes users and groups), but the implementation only tests users. If fully qualified names also apply to groups, this test should be expanded to cover them. Otherwise, the test should be renamed to clarify it only tests users.
| @pytest.mark.topology(KnownTopologyGroup.AnyProvider) | ||
| @pytest.mark.preferred_topology(KnownTopology.LDAP) | ||
| def test_memcache__all_caches_disabled_and_all_lookups_fails(client: Client, provider: GenericProvider): | ||
| def test_memcache__lookup_objects_by_name_with_case_sensitive_false(client: Client, provider: GenericProvider): |
There was a problem hiding this comment.
There are multiple contradictions regarding the case_sensitive setting in this test. The function name suggests it's false, but the docstring and implementation use and test for case_sensitive=True. The function name should be corrected to match the implementation and avoid confusion.
| def test_memcache__lookup_objects_by_name_with_case_sensitive_false(client: Client, provider: GenericProvider): | |
| def test_memcache__lookup_objects_by_name_with_case_sensitive_true(client: Client, provider: GenericProvider): |
|
|
||
| client.sssd.domain["case_sensitive"] = "false" | ||
| client.sssd.domain["ldap_id_mapping"] = "false" | ||
| 1. Objects are not found |
There was a problem hiding this comment.
The expected result in the docstring is "Objects are not found", but the test implementation correctly asserts that they are found. The docstring should be updated to match the test's actual behavior and intent.
| 1. Objects are not found | |
| 1. Objects are found |
danlavu
force-pushed
the
tests-rm-memcache
branch
from
88f67f3 to
4164a67
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for ids in expected_groups: | ||
| for group in objects.get("groups", []): | ||
| if group.name == ids: | ||
| _result_initgroup = client.tools.getent.initgroups(user.name) |
Check notice
Code scanning / CodeQL
Unused local variable Note test
danlavu
force-pushed
the
tests-rm-memcache
branch
from
4164a67 to
92e2433
Compare
|
|
||
| from __future__ import annotations | ||
|
|
||
| import struct |
Check notice
Code scanning / CodeQL
Unused import Note test
danlavu
force-pushed
the
tests-rm-memcache
branch
from
92e2433 to
e451914
Compare
1 participant
No description provided.