TESTS: Refactor memcache tests #7889

andreboscatto · 2025-03-19T21:42:49Z

Refactored the entire memcache tests according to the new testing plan. Improved the tests using parametrization. Removed redundant tests and included the new ones.

andreboscatto · 2025-03-23T21:25:38Z

Need help with:

test_memcache__lookup_users_by_fully_qualified_names for all providers

FAILED tests/test_memcache.py::test_memcache__lookup_users_by_fully_qualified_names[with_fqdn] (ad) - AssertionError: 1 - user1@test should be found when use_fully_qualified_names is True
FAILED tests/test_memcache.py::test_memcache__lookup_users_by_fully_qualified_names[without_fqdn] (ad) - AssertionError: 3 - user1@test should be found when use_fully_qualified_names is False

test_memcache__lookup_user_membership_with_cache -> Samba and AD are failing

FAILED tests/test_memcache.py::test_memcache__lookup_user_membership_with_cache[by_group_name] (ad) - AssertionError: `user1` should be found
FAILED tests/test_memcache.py::test_memcache__lookup_user_membership_with_cache[by_group_id] (ad) - AssertionError: `user1` should be found
FAILED tests/test_memcache.py::test_memcache__lookup_user_membership_with_cache[by_group_name] (samba) - AssertionError: `user1` should be found
FAILED tests/test_memcache.py::test_memcache__lookup_user_membership_with_cache[by_group_id] (samba) - AssertionError: `user1` should be found

test_memcache__case_sensitivity_affects_cache_lookup -> Failing with LDAP and IPA for case sensitive = false

tests/test_memcache.py::test_memcache__case_sensitivity_affects_cache_lookup[case_insensitive] (ipa) FAILED
FAILED tests/test_memcache.py::test_memcache__case_sensitivity_affects_cache_lookup[case_insensitive] (ipa) - AssertionError: Group info should be cached regardless of case.
tests/test_memcache.py::test_memcache__case_sensitivity_affects_cache_lookup[case_insensitive] (ldap) FAILED
FAILED tests/test_memcache.py::test_memcache__case_sensitivity_affects_cache_lookup[case_insensitive] (ldap) - AssertionError: Group info should be cached regardless of case.

kopecmartin · 2025-03-24T07:35:16Z

src/tests/system/tests/test_memcache.py

-"""
-SSSD In-Memory Cache (memcache) Test Cases.
-
-:requirement: IDM-SSSD-REQ: Client side performance improvements


some of the jobs failed on Check polarion metadata task .. probably because it didn't find test results it was looking for most likely because of the fact that this is deleted.

ValueError: Required field 'requirement' is missing for 'tests/test_memcache.py::test_memcache__user_group_initigroup_cache_alternately_disabled[with_memcache-users] (ipa)'

https://github.com/SSSD/sssd/actions/runs/14023157007/job/39257996729?pr=7889

@danlavu can you help me with that?

Right now, lets change the requirement name to something else, :requirement: memcache

src/tests/system/tests/test_memcache.py

alexey-tikhonov · 2025-04-01T11:46:53Z

All system tests are red, including some of tests being added.

src/tests/system/tests/test_memcache.py

jakub-vavra-cz · 2025-04-02T06:46:06Z

src/tests/system/tests/test_memcache.py

-        result = client.tools.id("user1@test")
-        assert result is not None, "User user1@test was not found using id"
-        assert result.user.name == "user1@test", f"User {result.user.name} has incorrect name, user1@test expected"
+    if lookup_type in ("users", "users_and_groups") and not memcache:


This is ugly, it would be much better to have multiple stanalone tests.

I did a bit of refactoring there, but not as much as you would like to. I hope we can find a middle ground.

src/tests/system/tests/test_memcache.py

jakub-vavra-cz

It looks like the testcases are merged/parametrized too much making them hard to read. I suggest splitting the ones that have this anti-pattern

if x:
    assert abc
    assert def
else:
    assert ghi
    assert jkl

In ideal case the asserts match 1:1 with steps/expected results.

jakub-vavra-cz · 2025-04-02T08:51:24Z

src/tests/system/tests/test_memcache.py

-    gresult = client.tools.getent.group("group1")
-    assert gresult is not None, "Group group1 is not found using getent"
-    assert gresult.gid == 10001, f"Group gid {gresult.gid} is incorrect, 10001 expected"
+    if by_gid:


This code branching suggests there should be (at least) two testcases instead.

I agree with improving the readability of the tests, but I disagree breaking this test into 2.

Something nicer would be:

if search_type["by_gid"]: ... if search_type["by_name"]:

wdyt?

src/tests/system/tests/test_memcache.py

alexey-tikhonov · 2025-04-02T11:21:40Z

https://github.com/SSSD/sssd/actions/runs/14217973656/job/39838952159?pr=7889

Found 4 errors in 1 file (checked 23 source files)

andreboscatto · 2025-04-02T11:27:56Z

https://github.com/SSSD/sssd/actions/runs/14217973656/job/39838952159?pr=7889
Found 4 errors in 1 file (checked 23 source files)

I am investigating that because it doesn't make sense what is happening. It is literally the same call I am using on other places.

Refactored the entire memcache tests according to the new testing plan. Improved the tests using parametrization. Removed redundant tests and included the new ones.

jakub-vavra-cz

Parametrizing test to a single assert selected by different conditions just makes it harder to read and increases overhead of the setup/teardown in comparison with making the test with more steps or keeping multiple simple and readable tests . I just can't agree with going this direction.

jakub-vavra-cz · 2025-04-07T12:39:07Z

src/tests/system/tests/test_memcache.py

+    if memcache:
+        if lookup_type in ("users"):
+            assert (
+                client.tools.getent.passwd("user1") is not None
+            ), "`user1` should still be found after stopping SSSD."
+        if lookup_type in ("groups"):
+            assert (
+                client.tools.getent.group("group1") is not None
+            ), "`group1` should still be found after stopping SSSD."
+    elif not memcache:
+        if lookup_type in ("users"):
+            assert client.tools.getent.passwd("user1") is None, "`user1` should NOT be found after stopping SSSD."
+        if lookup_type in ("groups"):
+            assert client.tools.getent.group("group1") is None, "`group1` should NOT be found after stopping SSSD."


This is still unsightly. How about rewriting it like this (maybe parametrize only "lookup_type", ["users", "groups"]?

sssd is running and user and group is found.

sssd is stopped with memcache and user and group is found.

sssd is started again without memcache and user and group is found.

sssd is stopped without memcache and user and group is not found.

If You make it in parametrized to 4 tests it will be all of the setup and teardown will be run 4 times for each of the provider making the overhead costly and the test less readable. It might be better to have more steps it the test instead at the cost that firs failure will stop the test.

alexey-tikhonov · 2025-04-09T14:29:45Z

@andreboscatto, could you please rebase?

andreboscatto marked this pull request as draft March 19, 2025 21:42

andreboscatto added Tests branch: sssd-2-9 labels Mar 19, 2025

andreboscatto added the Help wanted Extra attention is needed label Mar 23, 2025

andreboscatto force-pushed the testMemcached branch 2 times, most recently from 8decb00 to 1391c5d Compare March 23, 2025 21:26

kopecmartin reviewed Mar 24, 2025

View reviewed changes

andreboscatto force-pushed the testMemcached branch 2 times, most recently from be9858f to 52665e0 Compare March 27, 2025 07:19

github-advanced-security bot found potential problems Mar 27, 2025

View reviewed changes

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

andreboscatto force-pushed the testMemcached branch 2 times, most recently from c143bf1 to 14ba349 Compare March 29, 2025 08:30

github-advanced-security bot found potential problems Mar 29, 2025

View reviewed changes

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

andreboscatto added Waiting for review and removed branch: sssd-2-9 Help wanted Extra attention is needed labels Mar 29, 2025

andreboscatto force-pushed the testMemcached branch from 14ba349 to 60d6b81 Compare March 29, 2025 14:22

andreboscatto marked this pull request as ready for review March 29, 2025 14:22

andreboscatto force-pushed the testMemcached branch 2 times, most recently from 29b65f2 to 4b4241d Compare March 29, 2025 14:58

alexey-tikhonov added Changes requested and removed Waiting for review labels Apr 1, 2025

andreboscatto requested review from alexey-tikhonov and jakub-vavra-cz April 1, 2025 12:38

andreboscatto assigned alexey-tikhonov and jakub-vavra-cz Apr 1, 2025

andreboscatto force-pushed the testMemcached branch from 4b4241d to 38059b2 Compare April 1, 2025 12:53

jakub-vavra-cz reviewed Apr 2, 2025

View reviewed changes

src/tests/system/tests/test_memcache.py Outdated Show resolved Hide resolved

jakub-vavra-cz reviewed Apr 2, 2025

View reviewed changes

src/tests/system/tests/test_memcache.py Outdated Show resolved Hide resolved

jakub-vavra-cz reviewed Apr 2, 2025

View reviewed changes

src/tests/system/tests/test_memcache.py Outdated Show resolved Hide resolved

andreboscatto force-pushed the testMemcached branch from 38059b2 to a5d3afa Compare April 2, 2025 07:19

jakub-vavra-cz requested changes Apr 2, 2025

View reviewed changes

andreboscatto force-pushed the testMemcached branch from a5d3afa to 2a14e9c Compare April 2, 2025 10:09

github-advanced-security bot found potential problems Apr 2, 2025

View reviewed changes

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

andreboscatto force-pushed the testMemcached branch from 2a14e9c to df37f34 Compare April 2, 2025 10:59

andreboscatto force-pushed the testMemcached branch 2 times, most recently from 57875cc to a3df52d Compare April 2, 2025 14:00

andreboscatto requested a review from jakub-vavra-cz April 2, 2025 14:58

andreboscatto added Waiting for review and removed Changes requested labels Apr 2, 2025

TESTS: Refactor memcache tests

cb3ca3a

Refactored the entire memcache tests according to the new testing plan. Improved the tests using parametrization. Removed redundant tests and included the new ones.

andreboscatto force-pushed the testMemcached branch from a3df52d to cb3ca3a Compare April 7, 2025 12:14

jakub-vavra-cz requested changes Apr 7, 2025

View reviewed changes

andreboscatto added Changes requested and removed Waiting for review labels Apr 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TESTS: Refactor memcache tests #7889

TESTS: Refactor memcache tests #7889

andreboscatto commented Mar 19, 2025

andreboscatto commented Mar 23, 2025 •

edited

Loading

kopecmartin Mar 24, 2025

andreboscatto Mar 29, 2025

danlavu Apr 5, 2025

andreboscatto Apr 7, 2025

alexey-tikhonov commented Apr 1, 2025

jakub-vavra-cz Apr 2, 2025

andreboscatto Apr 2, 2025

jakub-vavra-cz left a comment

jakub-vavra-cz Apr 2, 2025

andreboscatto Apr 2, 2025 •

edited

Loading

alexey-tikhonov commented Apr 2, 2025 •

edited

Loading

andreboscatto commented Apr 2, 2025

jakub-vavra-cz left a comment

jakub-vavra-cz Apr 7, 2025

alexey-tikhonov commented Apr 9, 2025

TESTS: Refactor memcache tests #7889

Are you sure you want to change the base?

TESTS: Refactor memcache tests #7889

Conversation

andreboscatto commented Mar 19, 2025

andreboscatto commented Mar 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-tikhonov commented Apr 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakub-vavra-cz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreboscatto Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

alexey-tikhonov commented Apr 2, 2025 • edited Loading

andreboscatto commented Apr 2, 2025

jakub-vavra-cz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-tikhonov commented Apr 9, 2025

andreboscatto commented Mar 23, 2025 •

edited

Loading

andreboscatto Apr 2, 2025 •

edited

Loading

alexey-tikhonov commented Apr 2, 2025 •

edited

Loading