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

test: add insights-client test case for tags #293

Merged
merged 1 commit into from
Oct 22, 2024
Merged

test: add insights-client test case for tags #293

merged 1 commit into from
Oct 22, 2024

Conversation

zhangqianqian
Copy link

This PR test how the tags generated, and check the tags on inventory.

Copy link
Contributor

@m-horky m-horky left a comment

Choose a reason for hiding this comment

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

Generally LGTM from my point of view. Also, there's a typo in the title, clilent.

integration-tests/test_tags.py Outdated Show resolved Hide resolved
@zhangqianqian zhangqianqian changed the title test: add insights-clilent test case for tags test: add insights-client test case for tags Sep 27, 2024
@zhangqianqian
Copy link
Author

Generally LGTM from my point of view. Also, there's a typo in the title, clilent.

Thanks, has modified the PR title. @m-horky would you pls help merge it?

integration-tests/test_tags.py Outdated Show resolved Hide resolved
integration-tests/test_tags.py Outdated Show resolved Hide resolved
integration-tests/test_tags.py Outdated Show resolved Hide resolved
@zhangqianqian
Copy link
Author

@m-horky modified the PR again as your suggestions. pls merge it.

Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

Thanks for the test! It seems to work fine, there are few small things to fix.


Since this is a new test, please write a proper docstring for it following the format of Testimony [1]; see also the internal CCT-657 and all the recent PRs done by Zdenek on the topic.

[1] https://testimony-qe.readthedocs.io/en/stable/


There is a lot of repetition of the path /etc/insights-client/tags.yaml all around the test, and lots of manual I/O: let's simplify it using pathlib.Path; declate it as global variable in constants.py:

import pathlib

[...]
TAGS_FILE = pathlib.Path("/etc/insights-client/tags.yaml")

Then it can be used easily -- few examples:

    with contextlib.suppress(FileNotFoundError):
        TAGS_FILE.unlink()
[...]
        assert not TAGS_FILE.exists()
[...]
    with TAGS_FILE.open("r") as tags_yaml:

Do not forget to remove the tags file at the end of the test; to make sure to remove it even in case the test fails, you can use contextlib.ExitStack():

    with contextlib.ExitStack() as stack:
        # Run insights-client --group
        insights_client.run("--group=first_tag")
        stack.callback(os.remove, TAGS_FILE)

        [the rest of the test until the end]

@zhangqianqian
Copy link
Author

Thanks for the test! It seems to work fine, there are few small things to fix.

Since this is a new test, please write a proper docstring for it following the format of Testimony [1]; see also the internal CCT-657 and all the recent PRs done by Zdenek on the topic.

[1] https://testimony-qe.readthedocs.io/en/stable/

There is a lot of repetition of the path /etc/insights-client/tags.yaml all around the test, and lots of manual I/O: let's simplify it using pathlib.Path; declate it as global variable in constants.py:

import pathlib

[...]
TAGS_FILE = pathlib.Path("/etc/insights-client/tags.yaml")

Then it can be used easily -- few examples:

    with contextlib.suppress(FileNotFoundError):
        TAGS_FILE.unlink()
[...]
        assert not TAGS_FILE.exists()
[...]
    with TAGS_FILE.open("r") as tags_yaml:

Do not forget to remove the tags file at the end of the test; to make sure to remove it even in case the test fails, you can use contextlib.ExitStack():

    with contextlib.ExitStack() as stack:
        # Run insights-client --group
        insights_client.run("--group=first_tag")
        stack.callback(os.remove, TAGS_FILE)

        [the rest of the test until the end]

Thanks Pino.

Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

integration-tests/test_tags.py::test_tags PASSED                         [ 92%]

thanks!

@ptoscano ptoscano merged commit 2cde684 into master Oct 22, 2024
21 of 22 checks passed
@ptoscano ptoscano deleted the test_tag branch October 22, 2024 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants