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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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 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]

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