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: add 'expo_force' tests #7744

Open
wants to merge 1 commit into
base: sssd-2-9-4
Choose a base branch
from

Conversation

shridhargadekar
Copy link
Contributor

The new value for the ldap_pwmodify_mode option 'exop_force' is added to existing test. A new test to illustrate the different behavior of 'exop' and 'exop_force' is added.

Reviewed-by: Justin Stephenson [email protected]
Reviewed-by: Pavel Březina [email protected]
(cherry picked from commit deefe9a)

@@ -14,8 +16,8 @@

@pytest.mark.ticket(bz=[795044, 1695574])
@pytest.mark.importance("critical")
@pytest.mark.authentication
@pytest.mark.parametrize("modify_mode", ["exop", "ldap_modify"])
@pytest.mark.builtwith("exop_force")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thim might skip the test altogether when the exop_force is missing but other ways to change password are available..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which scenario would that be where exop_force is missing but other ways are available? The exop_foce is available in this branch.
What's your alternate suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can You provide a reference to the tickets introducing exop_force?

Easiest way would be creating standalone test with proper mark.ticket and requirement in metadata so we have the traceability in polarion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shared the details internally

src/tests/system/tests/test_ldap.py Outdated Show resolved Hide resolved
3. Set "passwordGraceLimit" to "0"
4. Add a user to LDAP
5. Wait until the password is expired
6. Set "ldap_pwmodify_mode"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, although I admit this will be more difficult due to the parametrization, but I'm sure you can find something more meaningful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to rearrange this. Keeping as it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about Specify the operation to be used to change the password in the parametrization?

src/tests/system/tests/test_ldap.py Outdated Show resolved Hide resolved
client.sssd.start()

rc, _, _, _ = client.auth.parametrize(method).password_with_output("user1", "Secret123")
assert rc == expected, err_msg
Copy link
Contributor

Choose a reason for hiding this comment

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

This could use line or two of explaining comment what is going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO steps and expectedresults already explain it, but if you see the need for any additional explanation, I will not prevent it

The new value for the ldap_pwmodify_mode option 'exop_force' is added to
existing test. A new test to illustrate the different behavior of 'exop'
and 'exop_force' is added.

Reviewed-by: Justin Stephenson <[email protected]>
Reviewed-by: Pavel Březina <[email protected]>
(cherry picked from commit deefe9a)
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the hard work and for taking all the feedback into account.

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.

Get this merged for now until exop_force detection is present and it can be updated.

@alexey-tikhonov
Copy link
Member

Get this merged for now until exop_force detection is present and it can be updated.

@shridhargadekar, @jakub-vavra-cz, IIUC, feature detection was merged, so PR can be updated?

Besides, what branches does it target?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted no-backport This should go to target branch only. Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants