tests: Add incomplete triples and complex hierarchy netgroup tests #8262
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 adds two valuable system tests for netgroups, covering incomplete triples and complex hierarchies. The tests are well-structured and improve coverage. I've identified a correctness issue in the assertions of test_netgroup__incomplete_triples and suggested a fix to make it more robust. Additionally, I've proposed refactoring the assertions in test_netgroups__complex_hierarchy for better conciseness and reliability. With these changes, the new tests will be excellent additions to the suite.
madhuriupadhye
force-pushed
the
netgroup1
branch
3 times, most recently
from
d9af156 to
002331b
Compare
| :customerscenario: False | ||
| """ | ||
| if not isinstance(provider, (LDAP, Samba, AD)): | ||
| raise ValueError("IPA does not support domain in netgroups") |
There was a problem hiding this comment.
Use pytest skip instead.
There was a problem hiding this comment.
updated in all test cases.
|
|
||
| # Level 1: Base netgroups with only triples (no nested members) | ||
| ng_base1 = provider.netgroup("ng-base1").add() | ||
| ng_base1.add_member(host="host1", user="user1", domain="ldap.test") |
There was a problem hiding this comment.
There is everywhere hardcoded ldap.test domain.
There was a problem hiding this comment.
Updated in all test cases.
Adds two new netgroup tests. 1. test_netgroup__incomplete_triples - Verifies netgroups with missing host/user/domain fields work correctly 2. test_netgroups__complex_hierarchy - Verifies multi-level nested netgroups resolve inherited members correctly Signed-off-by: Madhuri Upadhye <[email protected]>
madhuriupadhye
force-pushed
the
netgroup1
branch
from
002331b to
6e83b81
Compare
Update all hardcoded 'ldap.test` to provider.domain. Replaced all occurrences of raise ValueError with pytest.skip() Signed-off-by: Madhuri Upadhye <[email protected]>
madhuriupadhye
force-pushed
the
netgroup1
branch
from
6e83b81 to
58967fe
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
Thanks for your work on this. I noticed the commit structure, and I think we need to clean it up before merging to maintain a clear git history and simplify cherry-picking for maintenance branches.
Commit number 2 changes code introduced in commit number 1 and it also refactors already existing code. My suggestion would be to:
- Commit 1: pure refactoring of the original, existing tests. This commit must pass tests on its own.
- Commit 2: add the two new test cases using the new, refactored structure.
| if not isinstance(provider, (LDAP, Samba, AD)): | ||
| raise ValueError("IPA does not support domain in netgroups") | ||
| pytest.skip("IPA does not support domain in netgroups") |
There was a problem hiding this comment.
If the marker topology is correctly written then there's no need for this code as IPA topology will not be run at all. I'd strongly suggest you to remove these lines and similar ones
| 4. Create a netgroup with only domain | ||
| 5. Create a netgroup with missing host | ||
| 6. Create a netgroup with missing user | ||
| 7. Start SSSD |
There was a problem hiding this comment.
You are missing Create a netgroup with missing domain
madhuriupadhye
force-pushed
the
netgroup1
branch
6 times, most recently
from
58d8c52 to
84ad58e
Compare
Fix mypy type checking errors and improve test assertions. Changes: - Update provider type annotations from GenericProvider to AD | LDAP | Samba for tests that use the domain parameter in add_member() - Pass string names instead of objects for ng and user parameters in add_member() calls to avoid union type incompatibility errors - Rename result variables (passwd_result, group_result) to avoid type conflicts when reusing variable for different return types - Add descriptive assertion failure messages for easier debugging Signed-off-by: Madhuri Upadhye <[email protected]>
madhuriupadhye
force-pushed
the
netgroup1
branch
from
84ad58e to
121f7bb
Compare
4 participants
Adds two new netgroup tests.
test_netgroup__incomplete_triples - Verifies netgroups with missing host/user/domain fields work correctly
test_netgroups__complex_hierarchy - Verifies multi-level nested netgroups resolve inherited members correctly