rewriting memcache tests by danlavu · Pull Request #8485 · SSSD/sssd

danlavu · 2026-02-27T07:57:40Z

No description provided.

gemini-code-assist

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.

src/tests/system/tests/test_memcache.py

madhuriupadhye · 2026-03-11T07:23:52Z

@danlavu, can you please check,
ValueError: Required field 'customerscenario' is missing for 'tests/test_memcache.py::test_memcache__lookup_objects_by_name[users] (ldap)'

danlavu · 2026-03-12T01:18:05Z

@madhuriupadhye @andreboscatto I suggest reviewing this PR using the split diff view and not the unified diff.

andreboscatto · 2026-03-12T15:07:59Z

@madhuriupadhye @andreboscatto I suggest reviewing this PR using the split diff view and not the unified diff.

Indeed, I started reviewing the old document, understanding each case and looking at yours. Doing that side by side is helpful.

danlavu · 2026-03-13T16:52:35Z

@danlavu, can you please check, ValueError: Required field 'customerscenario' is missing for 'tests/test_memcache.py::test_memcache__lookup_objects_by_name[users] (ldap)'

fixed.

madhuriupadhye · 2026-03-16T12:00:41Z

src/tests/system/tests/test_memcache.py

+    Each cache type the user membership is checked differently. For 'users' cache, it will use 'id' and check the
+    memberof attribute. For 'groups' cache, it will use 'getent group' and check the members attribute. For
+    'initgroups' cache, it will use 'getent initgroups' and check the memberof attribute with group ids. Importantly,
+    for 'initgroups' the results are ids; using the user_map, a lookup is performed to get the correct GIDs.DD


extra DD at the end of line, ...to get the correct GIDs..

madhuriupadhye · 2026-03-16T12:03:04Z

src/tests/system/tests/test_memcache.py

+    Check the existence of objects, either users, groups, or initgroups.
+
+    This is a helper function to parameterize the memcache test. The assertions for each
+    cache time are different. Looking up 'users, will use 'id', groups will use 'getent group',


Please update to Looking up 'users', will use

cache time or cache type?

madhuriupadhye · 2026-03-16T12:06:13Z

src/tests/system/tests/test_memcache.py

+):
    """
-    :title: Invalidate all parts of cache after SSSD is stopped
+    :title: Lookup objects and changing when the cache is invalidated, before or after stooping sssd


**stopping** sssd.

madhuriupadhye · 2026-03-16T12:08:47Z

src/tests/system/tests/test_memcache.py

-@pytest.mark.preferred_topology(KnownTopology.LDAP)
-def test_memcache__all_caches_disabled_and_all_lookups_fails(client: Client, provider: GenericProvider):
+    :param provider: Known topology.
+    :type provider: KnownTopology


wrong parameter type,
:type provider: GenericProvider

madhuriupadhye · 2026-03-16T12:15:25Z

src/tests/system/tests/test_memcache.py

-        7. User is not found
-        8. Group is not found
-    :customerscenario: False
+        2. Users are found


Replace 2. to 1..

madhuriupadhye · 2026-03-16T12:20:05Z

src/tests/system/tests/test_memcache.py

 @pytest.mark.topology(KnownTopologyGroup.AnyProvider)
 @pytest.mark.preferred_topology(KnownTopology.LDAP)
-def test_memcache__invalidate_user_cache_after_stop(client: Client, provider: GenericProvider):
+def test_memcache__lookup_users_by_name_with_case_sensitive_false(client: Client, provider: GenericProvider):


Name suggests “case_sensitive false”, but the test sets case_sensitive = "True" and checks that uppercase lookup fails. So either rename (e.g. to something like ...when_case_sensitive_true) or fix the docstring/title so they clearly describe “when case_sensitive is True, different case is not found”.

To even add more confusion, the test use to be called case_insensitivity

madhuriupadhye · 2026-03-16T12:22:34Z

src/tests/system/tests/test_memcache.py

+    :type cache: str
+    :param id: Lookup object by id, default is False
+    :type id: bool
+    :return:


return: is empty. Either document “None” or remove the line.

madhuriupadhye · 2026-03-16T12:25:38Z

please check,
ValueError: Number of steps and results do not match in idm-sssd-tc::tests/test_memcache.py::test_memcache__lookup_objects_by_id[users] (ldap)

289: 2: Group membership is correct -> 2. Group membership is correct

andreboscatto · 2026-03-16T23:03:02Z

src/tests/system/tests/test_memcache.py

+
+    if cache == "initgroups":
+        for initgroup in objects.get("users", []):
+            _group = objects.get("groups", [])[-1].name


getent.initgroups returns None if the user is not found, and then calling memberof() will throw an error instead of a graceful failure, right?

Even if that's not the case, the logic is verifying only the last group [-1], so, even tough the user is in other groups, the test pass. Shouldn't it verify if the user is NOT in any of the groups?

Please correct me if my understanding is wrong :)

So this logic is confusing. I pass in the created user objects, so those objects will always exist. The .get method returns a list of dict objects, and the value is another list. We iterate through the added group list and check whether the get entry results contain that value. It's the last item in the list because the first item is the user's gidnumber.

user1
[user_group, group1] # Since it has one group, it will iterate once

user2
[user_group, group1]
[user_group, group1, group2]

Not the prettiest logic.

Hmmm I see. Is it worth to add a comment there for future reference? Your call, I am fine with the explanation, but I want to avoid people changing anything in the test and having a hard time figuring out the logic.

andreboscatto · 2026-03-16T23:05:51Z

src/tests/system/tests/test_memcache.py

+
+    if cache == "initgroups":
+        for initgroup in objects.get("users", []):
+            result_initgroup = client.tools.getent.initgroups(str(initgroup.name))


Should we also check which groups are there or None is sufficient? I don't recall it, it was a long time ago I played with that attribute.

src/tests/system/tests/test_memcache.py

* parametrized test cases * added colliding hash test case * remove poor test scenarios

danlavu added the Tests label Feb 27, 2026

danlavu marked this pull request as draft February 27, 2026 07:57

gemini-code-assist bot reviewed Feb 27, 2026

View reviewed changes

danlavu force-pushed the tests-rm-memcache branch from 88f67f3 to 4164a67 Compare February 28, 2026 10:43

github-advanced-security bot found potential problems Feb 28, 2026

View reviewed changes

src/tests/system/tests/test_memcache.py Fixed Show fixed Hide fixed

danlavu force-pushed the tests-rm-memcache branch from 4164a67 to 92e2433 Compare March 1, 2026 00:20

github-advanced-security bot found potential problems Mar 1, 2026

View reviewed changes

src/tests/system/tests/test_memcache.py Fixed Show fixed Hide fixed

danlavu force-pushed the tests-rm-memcache branch 4 times, most recently from 800149e to 79780ef Compare March 2, 2026 22:48

danlavu added the Waiting for review label Mar 2, 2026

danlavu force-pushed the tests-rm-memcache branch 2 times, most recently from 36e1b79 to 757722f Compare March 3, 2026 01:32

github-advanced-security bot found potential problems Mar 3, 2026

View reviewed changes

src/tests/system/tests/test_memcache.py Fixed Show fixed Hide fixed

src/tests/system/tests/test_memcache.py Fixed Show fixed Hide fixed

danlavu force-pushed the tests-rm-memcache branch 2 times, most recently from bc7b7a2 to b484bad Compare March 3, 2026 03:41

danlavu marked this pull request as ready for review March 3, 2026 04:33

danlavu added backport-to-sssd-2-11 backport-to-sssd-2-12 labels Mar 3, 2026

kopecmartin requested review from andreboscatto and madhuriupadhye March 4, 2026 13:42

kopecmartin assigned madhuriupadhye and andreboscatto Mar 4, 2026

madhuriupadhye added Changes requested and removed Waiting for review labels Mar 11, 2026

danlavu force-pushed the tests-rm-memcache branch from b484bad to 29ad9e3 Compare March 11, 2026 14:12

madhuriupadhye requested changes Mar 16, 2026

View reviewed changes

andreboscatto requested changes Mar 16, 2026

View reviewed changes

Dan Lavu added 2 commits March 17, 2026 01:23

Reworked memcache tests

14f0fe0

* parametrized test cases * added colliding hash test case * remove poor test scenarios

removing intg memcache tests

3ab0a1e

danlavu force-pushed the tests-rm-memcache branch 2 times, most recently from f45a3e2 to 3ab0a1e Compare March 17, 2026 05:24

Conversation

danlavu commented Feb 27, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madhuriupadhye commented Mar 11, 2026

Uh oh!

danlavu commented Mar 12, 2026

Uh oh!

andreboscatto commented Mar 12, 2026

Uh oh!

danlavu commented Mar 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madhuriupadhye commented Mar 16, 2026 • edited by danlavu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

madhuriupadhye commented Mar 16, 2026 •

edited by danlavu

Loading