Skip to content

Conversation

@danlavu
Copy link

@danlavu danlavu commented Dec 8, 2025

No description provided.

Copy link

@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 refactors enumeration tests by removing the test_enumeration.py integration test and parameterizing several system tests in test_nss.py to run with enumeration enabled. While the refactoring of existing tests is a positive change, I have a significant concern about the removal of test_enumeration.py as it appears to result in a loss of test coverage for dynamic enumeration updates and the auto_private_groups feature. I've added a comment detailing this issue.

@danlavu danlavu changed the title Tests enumeration adding enumeration system tests Dec 9, 2025
@thalman thalman self-assigned this Dec 9, 2025
@thalman thalman self-requested a review December 9, 2025 16:28
@danlavu
Copy link
Author

danlavu commented Dec 9, 2025

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

The change looks good.

CI issues are not related to the change, but they do not seem to be just random failure

@danlavu danlavu force-pushed the tests-enumeration branch from b8e6573 to 93551e3 Compare January 6, 2026 16:35
Copy link
Contributor

@jakub-vavra-cz jakub-vavra-cz left a comment

Choose a reason for hiding this comment

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

LGTM

Dan Lavu added 2 commits January 8, 2026 13:54
Reviewed-by: Jakub Vávra <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Jakub Vávra <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@sssd-bot
Copy link

sssd-bot commented Jan 8, 2026

The pull request was accepted by @jakub-vavra-cz with the following PR CI status:


🟢 CodeFactor (success)
🟢 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-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 / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-42) (success)
🔴 ci / system (fedora-43) (failure)
🔴 ci / system (fedora-44) (failure)
➖ 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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants