Parametrize sssctl tests 4.

[dkarpele]

• Combine various sssctl tests to the single parametrized tests.

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

[danlavu]

LG2M

[alexey-tikhonov]

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

Imo, this parameterization is unreadable.

@pbrezina , what would you say?

[dkarpele]

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.

[danlavu]

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!"
    ```

[dkarpele]

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.

[alexey-tikhonov]

Up to you. I'll refrain.

[danlavu]

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

Fair, let's keep it as a string.

[danlavu]

@dkarpele thanks for the effort, but we discussed this in the standup today and would want the tests to be less confusing. Parametrize into three tests?

"does not exists" combining two tests
"File ownership and permission checked failed" combining two tests
"attribute 'searchbase' is not allowed in section" remains a single test

[dkarpele]

@dkarpele thanks for the effort, but we discussed this in the standup today and would want the tests to be less confusing. Parametrize into three tests?

In this case I think it makes no sense to parametrize because pairs of tests will be very different and have a lot of parameters. I will close this PR.