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

Parametrize sssctl tests 4. #7804

Closed
wants to merge 1 commit into from
Closed

Parametrize sssctl tests 4. #7804

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 55 additions & 114 deletions src/tests/system/tests/test_sssctl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,30 +525,6 @@ def test_sssctl__check_invalid_nss_section_name(client: Client):
assert "Section [nssx] is not allowed" in result.stdout, "Wrong error message on stdout"


@pytest.mark.tools
@pytest.mark.topology(KnownTopology.Client)
def test_sssctl__check_invalid_permission(client: Client):
"""
:title: Verify the permission of default configuration file
:setup:
1. Start SSSD, so default config is autimatically created
2. Change permission of default config file
:steps:
1. Call sssctl config-check
2. Check error message
:expectedresults:
1. config-check detects an error
2. Error message is properly set
:customerscenario: False
"""
client.sssd.common.local()
client.sssd.start()
client.fs.chmod("0777", "/etc/sssd/sssd.conf")
result = client.sssctl.config_check()
assert result.rc != 0, "Config-check did not detect misconfigured config"
assert "File ownership and permissions check failed" in result.stdout, "Wrong error message on stdout"


@pytest.mark.tools
@pytest.mark.topology(KnownTopology.Client)
def test_sssctl__check_missing_closing_bracket(client: Client):
Expand Down Expand Up @@ -767,84 +743,72 @@ def test_sssctl__check_auto_private_groups_in_child_domains(client: Client):
@pytest.mark.tools
@pytest.mark.ticket(bz=1723273)
@pytest.mark.topology(KnownTopology.Client)
def test_sssctl__check_non_default_config_location_missing_snippet_directory(client: Client):
"""
:title: sssctl config-check complains about non existing snippet directory when config is non default
:setup:
1. Copy sssd.conf file to different directory
:steps:
1. Call sssctl config-check on that different directory
2. Check error message
:expectedresults:
1. config-check failed
2. Error message is properly set
:customerscenario: True
"""
client.sssd.common.local()
client.sssd.config_apply()
client.fs.mkdir("/tmp/test/")
client.fs.copy("/etc/sssd/sssd.conf", "/tmp/test/")

result = client.sssctl.config_check(config="/tmp/test/sssd.conf")
assert result.rc != 0, "Config-check successfully finished"
assert "Directory /tmp/test/conf.d does not exist" in result.stdout, "Wrong error message on stdout"


@pytest.mark.tools
@pytest.mark.ticket(bz=1723273)
@pytest.mark.topology(KnownTopology.Client)
def test_sssctl__check_non_default_config_location_invalid_permission(client: Client):
"""
:title: sssctl config-check complains about proper permission when config is non default
:setup:
1. Copy sssd.conf file to different directory and set it wrong permission
:steps:
1. Call sssctl config-check on that different directory
2. Check error message
:expectedresults:
1. config-check failed
2. Error message is properly set
:customerscenario: True
"""
client.sssd.common.local()
client.sssd.config_apply()
client.fs.mkdir("/tmp/test/")
client.fs.copy("/etc/sssd/sssd.conf", "/tmp/test/sssd.conf", mode="777")

result = client.sssctl.config_check(config="/tmp/test/sssd.conf")
assert result.rc != 0, "Config-check successfully finished"
assert "File ownership and permissions check failed" in result.stdout, "Wrong error message on stdout"


@pytest.mark.tools
@pytest.mark.ticket(bz=1723273)
@pytest.mark.topology(KnownTopology.Client)
def test_sssctl__check_non_default_config_location_invalid_option_name(client: Client):
"""
:title: sssctl config-check detects typo in option name when config is non default
@pytest.mark.parametrize(
Copy link
Member

Choose a reason for hiding this comment

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

@dkarpele , @danlavu , what is the rationale behind this effort in general?

Imo, this parameterization is unreadable.

@pbrezina , what would you say?

Copy link
Contributor Author

@dkarpele dkarpele Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

From my point of view, some of the tests in test_sssctl.py just duplicate each other. They have exactly the same code with different values. This is the main case for pytest.parametrization. Duplicated code is difficult to maintain because if we need to change it, we need to update it in many places.

Copy link

@danlavu danlavu Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is hard to read. I much rather have something like this than four test cases, but that is from "test transformation trauma". ;)

We can shorten the names to have make a little more sense, and put some of the variable declarations in the test.

@pytest.mark.parametrize(
    "option,mode,path,path_mode,snippet,stdout",
    [
        ("enabled", "0600", "/tmp/test/", None, True, "Directory /does/not/exist does not exist"),
        (
            "enabled",
            "0600",
            "/tmp/test/",
            None,
            False,
            "Directory /tmp/test/conf.d does not exist",
        ),
        (
            "enabled",
            "0600",
            "/tmp/test/sssd.conf",
            "777",
            False,
            "File ownership and permissions check failed",
        ),
        ("enabled", "0777", "/tmp/test/", None, False, "File ownership and permissions check failed"),
        (
            "search_base",
            "0600",
            "/tmp/test/",
            None,
            False,
            "Attribute 'search_base' is not allowed in section 'domain/local'",
        ),
    ],
)
def test_sssctl__check_config_location_permissions(
    client: Client,
    option: str,
    mode: str,
    path: str,
    path_mode: str,
    snippet: bool,
    stdout: str,
):
    config: str = f"{path}/sssd.conf"
    snippet_path: str = ""
    if snippet:
        snippet_path = "/doest/not/exist"

    client.sssd.common.local()
    client.sssd.default_domain = "local"
    client.sssd.domain[option] = "true"
    client.sssd.config_apply(check_config=False)

    client.fs.mkdir("/tmp/test/")
    client.fs.chmod(mode, "/etc/sssd/sssd.conf")
    client.fs.copy("/etc/sssd/sssd.conf", path, mode=path_mode)

    result = client.sssctl.config_check(config=config, snippet=snippet_path)
    assert result.rc != 0, "Config-check successfully finished!"
    assert stdout in result.stdout, "Wrong error message on stdout!"
    ```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imho, This version is still complicated.
And imagine we have third, fourth value for snippet not only "/doest/not/exist":

if snippet: 
   snippet_path = "/doest/not/exist"

In this case just True/False is not enough.

Copy link
Member

Choose a reason for hiding this comment

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

Up to you. I'll refrain.

Copy link

Choose a reason for hiding this comment

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

In this case, just True/False is not enough.

Fair, let's keep it as a string.

"option,mode,path,path_mode,config,snippet,stdout",
[
("enabled", "0600", "/tmp/test/", None, None, "/does/not/exist", "Directory /does/not/exist does not exist"),
(
"enabled",
"0600",
"/tmp/test/",
None,
"/tmp/test/sssd.conf",
None,
"Directory /tmp/test/conf.d does not exist",
),
(
"enabled",
"0600",
"/tmp/test/sssd.conf",
"777",
"/tmp/test/sssd.conf",
None,
"File ownership and permissions check failed",
),
("enabled", "0777", "/tmp/test/", None, None, None, "File ownership and permissions check failed"),
(
"search_base",
"0600",
"/tmp/test/",
None,
"/tmp/test/sssd.conf",
None,
"Attribute 'search_base' is not allowed in section 'domain/local'",
),
],
)
def test_sssctl__check_config_location_permissions(
client: Client,
option: str,
mode: str,
path: str,
path_mode: str | None,
config: str | None,
snippet: str | None,
stdout: str,
):
"""
:title: sssctl config-checks validates configuration file path and permissions
:setup:
1. Copy sssd.conf file to different directory and mistype option name
1. Copy the configuration to a new path with different permissions
:steps:
1. Call sssctl config-check on that different directory
2. Check error message
1. sssctl validates the copy of the configuration
:expectedresults:
1. config-check failed
2. Error message is properly set
1. sssctl configuration check fails with the correct output
:customerscenario: True
"""
client.sssd.common.local()
client.sssd.default_domain = "local"
client.sssd.domain["search_base"] = "True"
client.sssd.domain[option] = "true"
client.sssd.config_apply(check_config=False)

client.fs.mkdir("/tmp/test/")
client.fs.copy("/etc/sssd/sssd.conf", "/tmp/test/")
client.fs.chmod(mode, "/etc/sssd/sssd.conf")
client.fs.copy("/etc/sssd/sssd.conf", path, mode=path_mode)

result = client.sssctl.config_check(config="/tmp/test/sssd.conf")
assert result.rc != 0, "Config-check successfully finished"
assert (
"Attribute 'search_base' is not allowed in section 'domain/local'" in result.stdout
), "Wrong error message on stdout"
result = client.sssctl.config_check(config=config, snippet=snippet)
assert result.rc != 0, "Config-check successfully finished!"
assert stdout in result.stdout, "Wrong error message on stdout!"


@pytest.mark.tools
Expand Down Expand Up @@ -875,29 +839,6 @@ def test_sssctl__check_non_default_config_location_with_snippet_directory(client
assert "Directory /tmp/test/conf.d does not exist" not in result.stdout, "Wrong error message on stdout"


@pytest.mark.tools
@pytest.mark.ticket(bz=1723273)
@pytest.mark.topology(KnownTopology.Client)
def test_sssctl__check_non_existing_snippet(client: Client):
"""
:title: sssctl config-check detects non existing snippet directory
:setup:
1. Start SSSD, so default config is autimatically created
:steps:
1. Call sssctl config-check with non existing snippet
2. Check error message
:expectedresults:
1. config-check failed
2. Error message is properly set
:customerscenario: True
"""
client.sssd.common.local()
client.sssd.start()
result = client.sssctl.config_check(snippet="/does/not/exist")
assert result.rc != 0, "Config-check successfully finished"
assert "Directory /does/not/exist does not exist" in result.stdout, "Wrong error message on stdout"


@pytest.mark.tools
@pytest.mark.ticket(bz=1294670)
@pytest.mark.topology(KnownTopology.LDAP)
Expand Down
Loading