[autobackport: sssd-2-11] adding enumeration system tests #8346
[autobackport: sssd-2-11] adding enumeration system tests #8346sssd-bot wants to merge 2 commits intoSSSD:sssd-2-11from
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 backport that replaces older integration tests for enumeration with new, parameterized system tests. This is a good improvement to the test suite, increasing coverage for enumeration scenarios. The changes are mostly correct, but I've found a couple of minor inconsistencies in type hints within the modified test file. Addressing these will improve code clarity and maintainability.
| @pytest.mark.parametrize("config", ["enumerate", "default"]) | ||
| @pytest.mark.preferred_topology(KnownTopology.LDAP) | ||
| def test_nss__filter_users(client: Client, provider: LDAP): | ||
| def test_nss__filter_users(client: Client, provider: LDAP, config: str): |
There was a problem hiding this comment.
The type hint for the provider argument is LDAP, but the test is marked with @pytest.mark.topology(KnownTopologyGroup.AnyProvider), which indicates it should be able to run with any provider. To align with the test's intended scope and improve type safety, the hint should be changed to the more general GenericProvider.
| def test_nss__filter_users(client: Client, provider: LDAP, config: str): | |
| def test_nss__filter_users(client: Client, provider: GenericProvider, config: str): |
| @pytest.mark.topology(KnownTopology.LDAP) | ||
| def test_nss__filters_cached(client: Client, provider: GenericProvider): | ||
| @pytest.mark.parametrize("config", ["enumerate", "default"]) | ||
| def test_nss__filters_cached(client: Client, provider: GenericProvider, config: str): |
There was a problem hiding this comment.
The type hint for the provider argument is GenericProvider, but the test is marked with @pytest.mark.topology(KnownTopology.LDAP) and the docstring explicitly states that this is an LDAP-only test. For better type clarity and correctness, the hint should be changed to the more specific LDAP.
| def test_nss__filters_cached(client: Client, provider: GenericProvider, config: str): | |
| def test_nss__filters_cached(client: Client, provider: LDAP, config: str): |
|
The ci failures are unrelated, but the gemini review comments seem valid. |
jakub-vavra-cz
left a comment
jakub-vavra-cz
left a comment
There was a problem hiding this comment.
See gemini comment
3 participants
This is an automatic backport of PR#8280 adding enumeration system tests to branch sssd-2-11, created by @danlavu.
Please make sure this backport is correct.
Note
The commits were cherry-picked without conflicts.
You can push changes to this pull request
Original commits
160bbb3 - adding parametrized enumeration enabled tests
5b5dce2 - removing intg enumeration tests.
Backported commits
Original Pull Request Body