tests: updating pysss_nss_idmap error with more detail - #8797
Conversation
danlavu
added
Tests
Trivial
![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 improves assertion error messages in the test_pysss_nss_idmap.py test suite by including the actual command output on failure. However, a copy-paste error was introduced in test_ignore_unreadable_references where a successful ID lookup assertion was incorrectly changed to expect an empty dictionary and a mismatched error message. This issue should be corrected as suggested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
b43ac84 to
187e6c8
Compare
sssd-bot
left a comment
sssd-bot
left a comment
There was a problem hiding this comment.
Review done using Claude Code / claude-opus-4-6
Functional Issues
-
getidbysidassertion change contradicts test docstring (src/tests/system/tests/test_pysss_nss_idmap.py:391-394): Thetest_ignore_unreadable_referencestest docstring at lines 330-335 states that step 3 ("Lookup ID and name by group SID") has expected result "Group found". However, thegetidbysidassertion was changed to expect an empty dict{}, meaning the group is not found. If the behavioral change is intentional (as the author states), the docstring must be updated to reflect this. Currently the test documentation says the opposite of what the test asserts.Suggested docstring update (around line 334):
:steps: 1. Lookup group SID by name, ID, GID 2. Lookup group SID by UID 3. Lookup ID by group SID 4. Lookup name by group SID :expectedresults: 1. Group found 2. Group not found 3. Group not found 4. Group found -
Error message still says "when queried as UID" instead of "when queried as SID" (
src/tests/system/tests/test_pysss_nss_idmap.py:394): Reviewer alexey-tikhonov pointed out in the latest comment that the error message"Unexpectedly found ID for group SID '{group_id}' when queried as UID: ..."should say "when queried as SID" sincegetidbysidqueries by SID, not by UID. This has not been addressed yet. -
Error message uses
group_idinstead ofgroup_sid(src/tests/system/tests/test_pysss_nss_idmap.py:394): The message reads"Unexpectedly found ID for group SID '{group_id}'"butgroup_idis a numeric GID, not a SID string. Since the function being called isgetidbysid(group_sid), the message should referencegroup_sidto show what was actually queried. The corrected message should be:f"Unexpectedly found ID for group SID '{group_sid}' when queried as SID: {output.stdout.strip()}" -
Inconsistency:
getidbysidexpects empty butgetnamebysidstill expects success (src/tests/system/tests/test_pysss_nss_idmap.py:391-399): Right after asserting thatgetidbysid(group_sid)returns{}(group not found by SID), the test asserts thatgetnamebysid(group_sid)returns{group_sid: {"name": group.name, "type": 2}}(group found by the same SID). Both functions query the same SID. Ifgetidbysidtruly should return empty for an unreadable reference scenario, it seems odd thatgetnamebysidwould succeed with the same SID. Please clarify whether this inconsistency is expected behavior or whethergetnamebysidshould also be updated.
Nits & Non-functional Issues
-
Inconsistent update of negative-case error messages (
src/tests/system/tests/test_pysss_nss_idmap.py:389): Thegetsidbyuidassertion at line 389 intest_ignore_unreadable_referencesstill uses the old-style message format (f"'{group.name}' has wrong SID {group_sid}!"), while other similar negative-case assertions in the same PR were updated to the new descriptive format. For consistency, this should also be updated to something likef"Unexpectedly found SID for group '{group.name}' when queried as UID: {output.stdout.strip()}". -
Old-style error messages remain on success assertions: This PR only updates error messages for assertions that expect empty results (negative cases). Many success assertions still use the old-style
"has wrong SID"/"has wrong name"format (e.g., lines 96, 101, 130, 135, 191, 196, etc.) which don't include the actual output. The single exception istest_user_by_sidline 167, which was updated to the new format. The PR title says "updating pysss_nss_idmap error with more detail" -- consider whether the scope should extend to the remaining assertions for consistency, or add a note that those will be addressed separately. -
Missing
:relnote:tag consideration: This PR only modifies test code and does not change user-visible behavior, so no:relnote:tag is needed. This is fine.
Review of Existing Review Comments
-
gemini-code-assist[bot] flagged the
getidbysidassertion change as a "copy-paste error" / regression. The PR author responded that the empty-result expectation is intentional. If that is the case, this is not a copy-paste error but a deliberate test fix. However, the functional issues above (mismatched docstring, wrong variable in message, wrong "queried as" text) still need to be addressed. -
alexey-tikhonov (first comment) noted the error message should say "Unexpectedly found ID for group SID" rather than "Unexpectedly found SID for group ID". The author acknowledged this and said it was fixed. Looking at the current code at line 394, the message now reads
"Unexpectedly found ID for group SID", so this part has been addressed. -
alexey-tikhonov (second comment, most recent) noted that
"when queried as UID"should be"when queried as SID". This has not been addressed in the current code at line 394 -- the message still says "when queried as UID".
0b4fbe0 to
0ba9dca
Compare
Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Jakub Vávra <jvavra@redhat.com>
0ba9dca to
2d1bf29
Compare
4 participants
No description provided.