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: Ported Bash-krb-credential-cache to pytest #6555

Closed
wants to merge 3 commits into from

Conversation

SargunNarula
Copy link
Contributor

No description provided.

assert cmd2.returncode == 0, "/var/tmp/host_0 - No security context found"

else:
assert os_version != 6, "krb5rcachedir should be disabled"

Check warning

Code scanning / CodeQL

Redundant comparison

Test is always true, because of [this condition](1).
@jakub-vavra-cz jakub-vavra-cz self-assigned this Feb 2, 2023
:teardown:
1. Clear the /tmp/krb_cache directory.
"""
ldap_cmd = f"ldbsearch -H /var/lib/sss/db/config.ldb \
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not ldap but ldb_cmd.

assert cmd2.returncode == 0, f"{file}- no such file or directory."

@staticmethod
@pytest.mark.parametrize("ldap_krb5_setup", [pytest.param('False', id="1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please split this properly, the test is too long and has too many asserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought to parameterize this as the test revolves around checking the same scenarios for both true and false values of krb5_validate, If we split this in 2 test cases then it would require duplication of commands statements with their asserts respectively.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well making if request.node.name == 'test_0012_ldap_krb5[1]' makes it unsightly.
The result "auth success when krb_validate: False & fail if True" is completely confusing

I can not even guess what True/False and 0, 1 stands for in the test.

You can create _test_0012_ldap_krb5_helper function and share the common code parts there,
then You can have proper metadata with proper steps and results.

Always consider that description, setup, steps and expected results with should be good enough that someone can understand what is being tested and follow the test procedure manually without automation.

file = f"/var/log/sssd/sssd_{ds_instance_name}.log"
multihost.client[0].run_command(f"truncate -s 0 {file}")
client = sssdTools(multihost.client[0])
client.clear_sssd_cache()
Copy link
Contributor

Choose a reason for hiding this comment

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

This clears also logs, no need to call a truncate.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not clear secure log through.

3. SSSD domain log contains the required principal name message
"""
file = f"/var/log/sssd/sssd_{ds_instance_name}.log"
multihost.client[0].run_command(f"truncate -s 0 {file}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed due to clear cache being called later.

3. auth success when krb_validate: False & fail if True
4. auth success when krb_validate: False & success if True
"""
file = f"/var/log/sssd/sssd_{ds_instance_name}.log"

Check notice

Code scanning / CodeQL

Unused local variable

Variable file is not used.
client.sssd_conf(domain_section, domain_params, action="delete")
try:
client.clear_sssd_cache()
except SSSDException:
Copy link
Contributor

Choose a reason for hiding this comment

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

f"Option ldap_sasl_authid not found in {file}"

@staticmethod
def test_0014_bz1(multihost, bz_setup):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to add a descriptive test name instead of bz1

@jakub-vavra-cz
Copy link
Contributor

Hi @SargunNarula what is the progress on this?

@aborah-sudo
Copy link
Contributor

I am going to rise a new PR with modified content, as sargun is moving to another team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants