Skip to content

rewriting memcache tests#8485

Merged
danlavu merged 2 commits intoSSSD:masterfrom
danlavu:tests-rm-memcache
Apr 8, 2026
Merged

rewriting memcache tests#8485
danlavu merged 2 commits intoSSSD:masterfrom
danlavu:tests-rm-memcache

Conversation

@danlavu
Copy link
Copy Markdown

@danlavu danlavu commented Feb 27, 2026

No description provided.

@danlavu danlavu added the Tests label Feb 27, 2026
@danlavu danlavu marked this pull request as draft February 27, 2026 07:57
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Feb 27, 2026
View reviewed changes
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 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.

Comment thread src/tests/system/tests/test_memcache.py
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
github-advanced-security[bot]
github-advanced-security AI found potential problems Feb 28, 2026
View reviewed changes
Comment thread src/tests/system/tests/test_memcache.py Fixed
@danlavu danlavu force-pushed the tests-rm-memcache branch from 4164a67 to 92e2433 Compare March 1, 2026 00:20
github-advanced-security[bot]
github-advanced-security AI found potential problems Mar 1, 2026
View reviewed changes
Comment thread src/tests/system/tests/test_memcache.py Fixed
@danlavu danlavu force-pushed the tests-rm-memcache branch 4 times, most recently from 800149e to 79780ef Compare March 2, 2026 22:48
@danlavu 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]
github-advanced-security AI found potential problems Mar 3, 2026
View reviewed changes
Comment thread src/tests/system/tests/test_memcache.py Fixed
Comment thread src/tests/system/tests/test_memcache.py Fixed
@danlavu danlavu force-pushed the tests-rm-memcache branch 2 times, most recently from bc7b7a2 to b484bad Compare March 3, 2026 03:41
@danlavu danlavu marked this pull request as ready for review March 3, 2026 04:33
@madhuriupadhye
Copy link
Copy Markdown
Contributor

madhuriupadhye commented Mar 11, 2026

@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 danlavu force-pushed the tests-rm-memcache branch from b484bad to 29ad9e3 Compare March 11, 2026 14:12
@danlavu
Copy link
Copy Markdown
Author

danlavu commented Mar 12, 2026

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

@andreboscatto
Copy link
Copy Markdown
Contributor

andreboscatto commented Mar 12, 2026

@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
Copy link
Copy Markdown
Author

danlavu commented Mar 13, 2026

@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
madhuriupadhye requested changes Mar 16, 2026
View reviewed changes
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
@madhuriupadhye
Copy link
Copy Markdown
Contributor

madhuriupadhye commented Mar 16, 2026 •
edited by danlavu
Loading

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
andreboscatto requested changes Mar 16, 2026
View reviewed changes
Comment thread src/tests/system/tests/test_memcache.py
Comment thread src/tests/system/tests/test_memcache.py
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
@danlavu danlavu force-pushed the tests-rm-memcache branch 2 times, most recently from f45a3e2 to 3ab0a1e Compare March 17, 2026 05:24
@danlavu danlavu force-pushed the tests-rm-memcache branch from 3ab0a1e to c3b49a7 Compare March 26, 2026 16:48
madhuriupadhye
madhuriupadhye reviewed Mar 31, 2026
View reviewed changes
Comment thread src/tests/system/tests/test_memcache.py Outdated
@danlavu danlavu force-pushed the tests-rm-memcache branch from c3b49a7 to 926a6b0 Compare April 1, 2026 17:38
madhuriupadhye
madhuriupadhye reviewed Apr 2, 2026
View reviewed changes
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py Outdated
Comment thread src/tests/system/tests/test_memcache.py
Comment thread src/tests/system/tests/test_memcache.py
@madhuriupadhye
Copy link
Copy Markdown
Contributor

madhuriupadhye commented Apr 2, 2026

Please make sure it will be green in IDM-CI also.

andreboscatto
andreboscatto approved these changes Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@andreboscatto andreboscatto left a comment

Choose a reason for hiding this comment

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

Thanks, Dan! LGTM

@danlavu danlavu force-pushed the tests-rm-memcache branch from 926a6b0 to 7ac872e Compare April 7, 2026 15:25
Dan Lavu added 2 commits April 7, 2026 11:29
Reworked memcache tests
28b096a 
* parametrized test cases
* added colliding hash test case
* remove poor test scenarios
removing intg memcache tests
008ec15
@danlavu danlavu force-pushed the tests-rm-memcache branch from 7ac872e to 008ec15 Compare April 7, 2026 15:31
madhuriupadhye
madhuriupadhye approved these changes Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@madhuriupadhye madhuriupadhye left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks!

@danlavu danlavu merged commit 7d9bdd5 into SSSD:master Apr 8, 2026
29 of 31 checks passed
@sssd-bot
Copy link
Copy Markdown
Contributor

sssd-bot commented Apr 8, 2026

The pull request was accepted by @danlavu with the following PR CI status:

🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-44-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / intgcheck (fedora-45) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
🟢 ci / system (fedora-45) (success)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

This was referenced Apr 8, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andreboscatto andreboscatto andreboscatto approved these changes

@madhuriupadhye madhuriupadhye madhuriupadhye approved these changes

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@andreboscatto andreboscatto

@madhuriupadhye madhuriupadhye

Labels

Accepted backport-to-sssd-2-11 backport-to-sssd-2-12 Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danlavu @madhuriupadhye @andreboscatto @sssd-bot @github-advanced-security