Skip to content

Test migration sssldap#8120

Merged
alexey-tikhonov merged 1 commit intoSSSD:masterfrom
thalman:test_migration_sssldap
Nov 18, 2025
Merged

Test migration sssldap#8120
alexey-tikhonov merged 1 commit intoSSSD:masterfrom
thalman:test_migration_sssldap

Conversation

@thalman
Copy link
Contributor

@thalman thalman commented Sep 30, 2025

Here are missing tests from intg/test_ldap.py migrated to new framework in system/test_ldap.py converted by AI and fixed by me.

@thalman thalman requested a review from danlavu September 30, 2025 13:04
gemini-code-assist[bot]
gemini-code-assist bot reviewed Sep 30, 2025
View reviewed changes
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 migrates a substantial number of LDAP integration tests to the new system test framework. The new tests cover various nss configuration options in sssd.conf, such as home directory and shell overrides, fallbacks, and user/group filtering. The tests are well-structured and cover important scenarios. I've found a couple of issues in the shell-related tests where the assertions seem to be based on a misunderstanding of the allowed_shells option, leading to tests that validate incorrect behavior. My review includes suggestions to correct these assertions.

danlavu
danlavu requested changes Oct 1, 2025
View reviewed changes
Copy link

@danlavu danlavu left a comment

Choose a reason for hiding this comment

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

We have a test case in authentication for homedirs, but the more I think about it. I think we should create test_nss.py and move all the nss related test cases there and make them genericprovider and preferredtopology will be set to LDAP.

@thalman thalman requested a review from jakub-vavra-cz October 2, 2025 12:37
@thalman thalman force-pushed the test_migration_sssldap branch 2 times, most recently from 7c78b91 to e75e345 Compare October 10, 2025 16:08
@thalman
Copy link
Contributor Author

thalman commented Oct 10, 2025

I have moved nss test into separate new file and I expand the scope of other providers. But at the moment some of them fail for them. I will investigate and let you know.

@thalman thalman force-pushed the test_migration_sssldap branch from e75e345 to 427a9dc Compare October 13, 2025 12:08
@thalman
Copy link
Contributor Author

thalman commented Oct 13, 2025

I hope that I did not miss any of the comments. The PR is ready for second round.

@danlavu
Copy link

danlavu commented Oct 15, 2025

This looks great. Fix the linting errors. In another commit, do you want to move test_authentication__user_login_with_overriding_home_directory to test_nss.py ?

@thalman thalman force-pushed the test_migration_sssldap branch 2 times, most recently from 1be42d2 to a7e30e5 Compare October 17, 2025 11:38
@thalman
Copy link
Contributor Author

thalman commented Oct 17, 2025

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot reviewed Oct 17, 2025
View reviewed changes
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 migrates several LDAP-related tests from the old intg/test_ldap.py framework to the new system test framework in system/tests/test_nss.py. The changes include removing the old test file and its Makefile entry, and adding the new test file with the migrated and refactored tests. One test was also correctly relocated from test_authentication.py to test_nss.py. The migration is a good improvement. I've found one issue in a new test where time.sleep() calls were commented out, rendering the test's caching validation ineffective. I've provided suggestions to fix this.

@thalman thalman force-pushed the test_migration_sssldap branch 3 times, most recently from 64e9576 to d5383cb Compare October 17, 2025 12:33
@pbrezina pbrezina force-pushed the master branch 2 times, most recently from f5d64b3 to b854636 Compare November 4, 2025 14:27
jakub-vavra-cz
jakub-vavra-cz reviewed Nov 11, 2025
View reviewed changes
@danlavu
Copy link

danlavu commented Nov 11, 2025 

(.venv)  dlavu@rocket  ~/git/sssd   master  ~/bin/git_exists.sh src/tests/intg/test_ldap.py 
File src/tests/intg/test_ldap.py exists in branch master
File src/tests/intg/test_ldap.py exists in branch sssd-2-10
File src/tests/intg/test_ldap.py exists in branch sssd-2-11
File src/tests/intg/test_ldap.py exists in branch sssd-2-7
File src/tests/intg/test_ldap.py exists in branch sssd-2-8
File src/tests/intg/test_ldap.py exists in branch sssd-2-9
File src/tests/intg/test_ldap.py exists in branch sssd-2-9-4
(.venv)  dlavu@rocket  ~/git/sssd   master  ~/bin/git_exists.sh src/tests/system/tests/test_ldap.py 
File src/tests/system/tests/test_ldap.py exists in branch master
File src/tests/system/tests/test_ldap.py exists in branch sssd-2-10
File src/tests/system/tests/test_ldap.py exists in branch sssd-2-11
File src/tests/system/tests/test_ldap.py exists in branch sssd-2-7
File src/tests/system/tests/test_ldap.py exists in branch sssd-2-8
File src/tests/system/tests/test_ldap.py exists in branch sssd-2-9
File src/tests/system/tests/test_ldap.py exists in branch sssd-2-9-4

Adding all the branch labels where intg/test_ldap.py exists.

@thalman thalman force-pushed the test_migration_sssldap branch from d5383cb to 4914c7a Compare November 12, 2025 09:28
danlavu
danlavu reviewed Nov 12, 2025
View reviewed changes
Copy link

@danlavu danlavu left a comment

Choose a reason for hiding this comment

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

Here too, some assertions are missing error messages.

@thalman thalman force-pushed the test_migration_sssldap branch 2 times, most recently from 27ad915 to 556a607 Compare November 18, 2025 08:58
@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Nov 18, 2025 •
edited
Loading

This won't apply to (most of) older branches because intg-tests were already removed there (from most of them).

@thalman, once this gets merged to 'master', please open a backport that doesn't touch intg tests against sssd-2-11 and add 'backport-to-*'

@thalman
Copy link
Contributor Author

thalman commented Nov 18, 2025

Hi Guys,

I went trough the files and added error texts to all asserts.

Please review.

@thalman thalman force-pushed the test_migration_sssldap branch from 556a607 to 31f0338 Compare November 18, 2025 10:23
jakub-vavra-cz
jakub-vavra-cz approved these changes Nov 18, 2025
View reviewed changes
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.

danlavu
danlavu approved these changes Nov 18, 2025
View reviewed changes
Copy link

@danlavu danlavu left a comment

Choose a reason for hiding this comment

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

Thank you for adding the assertion messages, LGTM.

@thalman @sssd-bot
tests: Migrate missing tests to new framework
b4c1c29 
This PR implements tests from integ/test_ldap.py to new system tests

Reviewed-by: Dan Lavu <dlavu@redhat.com>
Reviewed-by: Jakub Vávra <jvavra@redhat.com>
@sssd-bot
Copy link
Contributor

sssd-bot commented Nov 18, 2025

The pull request was accepted by @alexey-tikhonov with the following PR CI status:

🟢 CodeFactor (success)
🟢 CodeQL (success)
NEUTRAL osh-diff-scan:fedora-rawhide-x86_64:upstream (neutral)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-41-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-41) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / prepare (success)
🔴 ci / system (centos-10) (failure)
🟢 ci / system (fedora-41) (success)
🟢 ci / system (fedora-42) (success)
⚪ ci / system (fedora-43) (cancelled)
⚪ ci / system (fedora-44) (cancelled)
➖ 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.

@sssd-bot sssd-bot force-pushed the test_migration_sssldap branch from 31f0338 to b4c1c29 Compare November 18, 2025 16:57
@alexey-tikhonov alexey-tikhonov merged commit 997ffd1 into SSSD:master Nov 18, 2025
13 of 17 checks passed
@danlavu danlavu mentioned this pull request Dec 9, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danlavu danlavu danlavu approved these changes

@jakub-vavra-cz jakub-vavra-cz jakub-vavra-cz 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

@danlavu danlavu

@jakub-vavra-cz jakub-vavra-cz

Labels

Accepted no-backport This should go to target branch only.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@thalman @danlavu @alexey-tikhonov @sssd-bot @jakub-vavra-cz