Test migration sssldap#8120
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 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
left a comment
danlavu
left a comment
There was a problem hiding this comment.
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.
7c78b91 to
e75e345
Compare
|
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. |
e75e345 to
427a9dc
Compare
|
I hope that I did not miss any of the comments. The PR is ready for second round. |
|
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 ? |
1be42d2 to
a7e30e5
Compare
|
/gemini review |
There was a problem hiding this comment.
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.
64e9576 to
d5383cb
Compare
f5d64b3 to
b854636
Compare
danlavu
added
backport-to-sssd-2-8
backport-to-sssd-2-9
backport-to-sssd-2-10
backport-to-sssd-2-9-4
Adding all the branch labels where intg/test_ldap.py exists. |
d5383cb to
4914c7a
Compare
danlavu
left a comment
danlavu
left a comment
There was a problem hiding this comment.
Here too, some assertions are missing error messages.
27ad915 to
556a607
Compare
alexey-tikhonov
added
no-backport
|
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-*' |
|
Hi Guys, I went trough the files and added error texts to all asserts. Please review. |
556a607 to
31f0338
Compare
danlavu
left a comment
danlavu
left a comment
There was a problem hiding this comment.
Thank you for adding the assertion messages, LGTM.
This PR implements tests from integ/test_ldap.py to new system tests Reviewed-by: Dan Lavu <[email protected]> Reviewed-by: Jakub Vávra <[email protected]>
31f0338 to
b4c1c29
Compare
5 participants
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.