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.

Sign up for GitHub

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

Jump to bottom

Parametrize sssctl tests. #7776

Closed
wants to merge 3 commits into from
Closed

Parametrize sssctl tests. #7776

wants to merge 3 commits into from
+161 −559

Conversation

dkarpele
Copy link
Contributor

@dkarpele dkarpele commented Dec 18, 2024 •
edited
Loading

  • Combine various sssctl tests to the single parametrized tests.
  1. test_sssctl__check_invalid_option_name_in_snippet merges tests:

test_sssctl__check_invalid_option_name_in_snippet
test_sssctl__check_invalid_section_in_name_in_snippet

  1. test_sssctl__check_invalid_section_name merges tests:

test_sssctl__check_missing_equal_sign
test_sssctl__check_invalid_id_provider
test_sssctl__check_missing_id_provider
test_sssctl__check_special_character_in_option_name
test_sssctl__check_special_character_in_section_name
test_sssctl__check_special_character_in_domain_name
test_sssctl__check_forward_slash_missing_in_domain_section
test_sssctl__check_invalid_sssd_section_name
test_sssctl__check_missing_closing_bracket
test_sssctl__check_missing_opening_bracket

  1. test_sssctl__check_attribute_not_allowed_in_sssd merges tests:

test_sssctl__check_misplaced_option
test_sssctl__check_ldap_host_object_class_not_allowed_in_sssd

  1. test_sssctl__check_config_location_permissions merges tests:

test_sssctl__check_non_existing_snippet
test_sssctl__check_non_default_config_location_missing_snippet_directory
test_sssctl__check_invalid_permission
test_sssctl__check_non_default_config_location_invalid_permission
test_sssctl__check_non_default_config_location_invalid_option_name

  1. Dropped tests:
    test_sssctl__check_invalid_pam_section_name (duplicate test_sssctl__check_invalid_sssd_section_name)
    test_sssctl__check_invalid_nss_section_name (duplicate test_sssctl__check_invalid_sssd_section_name)

@dkarpele dkarpele mentioned this pull request Dec 18, 2024
Closed
@dkarpele dkarpele force-pushed the dkarpele-param-sssctl-tests branch from c227175 to 41a2d38 Compare December 19, 2024 15:13
danlavu
danlavu previously approved these changes Jan 17, 2025
View reviewed changes
@danlavu danlavu self-requested a review January 17, 2025 04:05
danlavu
danlavu reviewed Jan 17, 2025
View reviewed changes
Copy link

@danlavu danlavu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks great; we can close this and open another PR for the code or update this one.

Github is strange for this; "tests' are approved, but the PR is not approved.

@dkarpele
Copy link
Contributor Author

This looks great; we can close this and open another PR for the code or update this one.

@danlavu I think this PR should be merged, not closed. It's only about tests parametrization. I don't have permissions to merge.
The remaining work will be done in separate PR.

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Jan 17, 2025 •
edited
Loading

Would it be possible to split into a set of patch - one per action described in the PR description?
It would make reading much easier.

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Jan 17, 2025 •
edited
Loading

'python-system-tests' fail 'blake':

-@pytest.mark.parametrize('fake', [""])
+@pytest.mark.parametrize("fake", [""])

but I questioned this param anyway.

@danlavu danlavu dismissed their stale review January 17, 2025 21:09

I thought it was a cursory overview of the plan, I need to give it a proper review.

@danlavu
Copy link

danlavu commented Jan 17, 2025

This looks great; we can close this and open another PR for the code or update this one.

@danlavu I think this PR should be merged, not closed. It's only about tests parametrization. I don't have permissions to merge. The remaining work will be done in separate PR.

I'm sorry, I thought this was a cursory "planning" PR. It still looks great but I need to give it a proper review.

danlavu
danlavu requested changes Jan 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.

Great job on the parameterization. Housekeeping hasn't been performed on this file yet. Thus, a lot of my comments are related to housekeeping and standardizing the code.

src/tests/system/tests/test_sssctl.py Outdated
@@ -150,7 +88,8 @@ def test_sssctl__handle_implicit_domain(client: Client):

@pytest.mark.ticket(bz=1902280)
@pytest.mark.topology(KnownTopology.LDAP)
def test_sssctl__reset_cached_timestamps_to_reflect_changes(client: Client, ldap: LDAP):
@pytest.mark.parametrize('fake', [""])
def test_sssctl__reset_cached_timestamps_to_reflect_changes(client: Client, ldap: LDAP, fake):
"""
:title: fix sssctl cache-expire to also reset cached timestamp
Copy link

@danlavu danlavu Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I will suggest docstring changes because this is part of the housekeeping effort. The title is overly complicated for what this test does.

:title: Clear the SSSD cache using sssctl command against rfc2307bis schema
        1. Create users and groups 
        2. Start SSSD
    :steps:
        1. Lookup group
        2. Remove user from group and clear the cache using sssctl 
        3. Lookup the group 
    :expectedresults:
        1. Group is found with the user as a member
        2. User is removed
        3. Group is found with no members

Using rfc2307bis is irrelevant to this test; I propose we remove it entirely, any objections @alexey-tikhonov ?

@dkarpele
Parametrize sssctl tests.
8b49f67 
- Combine various sssctl tests to the single parametrized tests.

Signed-off-by: Denis Karpelevich <dkarpele@redhat.com>
@dkarpele
Update snippet file mode to 640
9b8ab4a 
Signed-off-by: Denis Karpelevich <dkarpele@redhat.com>
@dkarpele
Fix review requests.
6c7715d 
- Change docstrings
- Add typing
- Fix typos

Signed-off-by: Denis Karpelevich <dkarpele@redhat.com>
@dkarpele dkarpele force-pushed the dkarpele-param-sssctl-tests branch from 41a2d38 to 6c7715d Compare January 18, 2025 17:18
@dkarpele
Copy link
Contributor Author

dkarpele commented Jan 18, 2025 •
edited
Loading

@danlavu @alexey-tikhonov Thank you for review!
I fixed your requests and I will split this PR into 4 separate PRs as Alexey suggested.
#7801 #7802 #7803 #7804
After that I will close this PR when we finish the discussion.

@jakub-vavra-cz
Copy link
Contributor

Does it bring any merit parametrize this aggressively?
The test cases were pretty simple and readable before which is not the case anymore after merging them this much together.

@danlavu
Copy link

danlavu commented Jan 20, 2025

Does it bring any merit parametrize this aggressively? The test cases were pretty simple and readable before which is not the case anymore after merging them this much together.

I like it, though I have faced hundreds of test cases where the description doesn't match what it does. I faced "trauma" when there were too many small test cases. Maybe we add in the :description: , the parameterization values, or add more docstrings?

@danlavu
Copy link

danlavu commented Jan 20, 2025

I believe, this can be closed now because of

@dkarpele
Copy link
Contributor Author

Ok, closing as there are no more comments

@dkarpele dkarpele closed this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danlavu danlavu danlavu requested changes

@alexey-tikhonov alexey-tikhonov alexey-tikhonov left review comments

Assignees

@danlavu danlavu

@alexey-tikhonov alexey-tikhonov

Labels
Changes requested Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dkarpele @alexey-tikhonov @danlavu @jakub-vavra-cz