Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: housekeeping, test_ldap.py #7506

Closed
wants to merge 1 commit into from

Conversation

danlavu
Copy link

@danlavu danlavu commented Jul 24, 2024

housekeeping, the following is looked at and may have been done:

  • fixed typos and standardized formatting
  • renamed test cases to improve the clarity of what the test does
  • improved docstring language, setup, steps and expected results
  • synced code with the docstring order
  • removed necessary configuration relevant to the test
  • added pytest.mark.importance to test cases

noteable changes:

  • removed lookup_user_default_naming_context_and_no_search_base because it is already covered by another test scenario added description to ppolicy tests, providing an explanation
  • removed enumeration configuration from tests where enumeration is not being tested
  • removed test case checking log messages for timeout

Reviewed-by: Alejandro López [email protected]
Reviewed-by: Anuj Borah [email protected]
Reviewed-by: Shridhar Gadekar [email protected]
(cherry picked from commit 8c19d7b)

@alexey-tikhonov
Copy link
Member

@danlavu, doesn't this de-sync 'master' and 'sssd-2-9' branches?

@danlavu
Copy link
Author

danlavu commented Jul 26, 2024

Yes, the ppolicy feature is not available in 2-9. Now, master contains

@pytest.mark.parametrize("use_ppolicy", ["true", "false"])

that we can skip the tests and leave the un-used test code. Do we want to remove it from 2-9 or add the marker to skip the tests in this branch?

housekeeping, the following is looked at and may have been done:
* fixed typos and standardized formatting
* renamed test cases to improve the clarity of what the test does
* improved docstring language, setup, steps and expected results
* synced code with the docstring order
* removed necessary configuration relevant to the test
* added pytest.mark.importance to test cases

noteable changes:
* removed lookup_user_default_naming_context_and_no_search_base because it is already covered by another test scenario
added description to ppolicy tests, providing an explanation
* removed enumeration configuration from tests where enumeration is not being tested
* removed test case checking log messages for timeout

Reviewed-by: Alejandro López <[email protected]>
Reviewed-by: Anuj Borah <[email protected]>
Reviewed-by: Shridhar Gadekar <[email protected]>
(cherry picked from commit 8c19d7b)
@danlavu danlavu force-pushed the cherry-pick-test-ldap branch from d206c65 to 6d807ec Compare July 26, 2024 23:43
@danlavu
Copy link
Author

danlavu commented Aug 2, 2024

@alexey-tikhonov ,

I spoke to Jakub. It will be harder to maintain if we drop these tests, so I've added them back in. You can see the tests are skipped for this branch.

tests/test_ldap.py::test_ldap__password_change_new_passwords_do_not_match_using_ppolicy[true-exop] (ldap) SKIPPED [ 47%]
tests/test_ldap.py::test_ldap__password_change_new_passwords_do_not_match_using_ppolicy[true-ldap_modify] (ldap) SKIPPED [ 47%]
tests/test_ldap.py::test_ldap__password_change_new_passwords_do_not_match_using_ppolicy[false-exop] (ldap) SKIPPED [ 48%]
tests/test_ldap.py::test_ldap__password_change_new_passwords_do_not_match_using_ppolicy[false-ldap_modify] (ldap) SKIPPED [ 48%]
tests/test_ldap.py::test_ldap__password_change_new_password_does_not_meet_complexity_requirements_using_ppolicy[true-exop] (ldap) SKIPPED [ 48%]
tests/test_ldap.py::test_ldap__password_change_new_password_does_not_meet_complexity_requirements_using_ppolicy[true-ldap_modify] (ldap) SKIPPED [ 49%]
tests/test_ldap.py::test_ldap__password_change_new_password_does_not_meet_complexity_requirements_using_ppolicy[false-exop] (ldap) SKIPPED [ 49%]
tests/test_ldap.py::test_ldap__password_change_new_password_does_not_meet_complexity_requirements_using_ppolicy[false-ldap_modify] (ldap) SKIPPED [ 49%]
tests/test_ldap.py::test_ldap__password_change_with_invalid_current_password_using_ppolicy[true-exop] (ldap) SKIPPED [ 49%]
tests/test_ldap.py::test_ldap__password_change_with_invalid_current_password_using_ppolicy[true-ldap_modify] (ldap) SKIPPED [ 50%]
tests/test_ldap.py::test_ldap__password_change_with_invalid_current_password_using_ppolicy[false-exop] (ldap) SKIPPED [ 50%]
tests/test_ldap.py::test_ldap__password_change_with_invalid_current_password_using_ppolicy[false-ldap_modify] (ldap) SKIPPED [ 50%]

@andreboscatto andreboscatto requested review from alexey-tikhonov and removed request for jakub-vavra-cz August 8, 2024 12:45
Copy link
Contributor

@aplopez aplopez left a comment

Choose a reason for hiding this comment

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

LGTM

@alexey-tikhonov alexey-tikhonov removed their request for review August 13, 2024 12:44
@alexey-tikhonov alexey-tikhonov removed their assignment Aug 13, 2024
@alexey-tikhonov alexey-tikhonov added the Ready to push Ready to push label Aug 13, 2024
@jakub-vavra-cz
Copy link
Contributor

Pushed PR: #7506

  • sssd-2-9
    • 1b6dce3 - tests: housekeeping, test_ldap.py

@danlavu danlavu deleted the cherry-pick-test-ldap branch November 4, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport This should go to target branch only. Pushed Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants